MSW mocks seem to "not consequently apply" "required:true" to anyOf elements that could be null

[snext-twilde]

Hi,

not an expert in this, but stumbled upon this behavior:

with config output: { ..., override: { required: true }, ... }

with openapi spec (from a pydantic/fastapi project with an Optional[str] attribute) :

"message": {
            "anyOf": [{ "type": "string" }, { "type": "null" }],
            "title": "Message"
          },

behavior: MSW mocks are null 50% of the time: faker.helpers.arrayElement([faker.string.alpha(20), null])

Now I'd like to have a mock that is always populated and have many of these fields where e.g. some data is only returned on some conditions and the difference between [] and null is relevant.

Would it be consequent in your understanding of "required" to make required:true force the null types out of the anyOf for mocks? Or am I looking at this the wrong way? LMK if you'd like a PR, in case you like the idea.

Best, Thomas

[melloware]

i would submit a PR for review. Both @AllieJonsson and @soartec-lab have made lots of MSW fixes recently so they can review!

[soartec-lab]

Hi @snext-twilde , We are currently rewriting the relevant source code in #1815, so we recommend that you wait for this PR to be merged before checking again.

[AllieJonsson]

The way the code works now, override.required only affects if the item is required or not, not if it is nullable or not (making a difference between undefined and null). I'm unsure if setting override.required = true should make the item not nullable as well?

Maybe we should add another override option for removing nullable from the mock? @soartec-lab

[snext-twilde]

@AllieJonsson agree, although connected in a way, these are definetly two pairs of shoes.

[soartec-lab]

@AllieJonsson @melloware

It is a make sense to have a property contain null only if nullable: true is specified like a below:

Pet:
      type: object
      required:
        - id
        - name
      properties:
        id:
          type: integer
          format: bigint
        name:
          type: string
          nullable: true

In this case, id is assumed to be string only, and name is assumed to be string or null.

[melloware]

Yes that does make sense

[snext-twilde]

true, but this - as far as I understand the approach - makes the msw mocks drastically less useful if you wrap your returned data in a generic "ApiResponse" that adds typical params like pagination, error code, error message and carries the data payload nested in a property that... yeah, in rare cases can be null (primarily in error cases).

For this scenario, the data payload is populated in only 50% of cases, in a nested payload structure with an optional parent property you see these properties only populated 25% of times. Using these mocks for tests means that you focus 50% on testing trivial error scenarios where no data is returned.

But thanks for the input, I can rephrase my question now: Would you consider it a useful feature to override nullable for specific properties in mock generation to force certain fields to always be populated?

[AllieJonsson]

I think that is something we should add!

[soartec-lab]

@snext-twilde In other words, there are cases where null is returned even if it is not defined when an error occurs, so you want to consider that case as well, right? That case is rare, so if you want to test it, explicitly override it with null or implement it separately. I would like the response automatically generated by orval to include null as a return candidate only when nulllabel: true is set.

[snext-twilde]

I think we still got a misunderstanding; to clarify: If an error occurs, my api response wrapper returns "null" as "data" and provides an error code among other things. So to orval this is simply a nullable property "data" and so it translates this to msw mocks with

data: faker.helpers.arrayElement([<nested elements with lots of well usable mock data>, null]).

But since this makes the top element in my nested object tree nullable, in 50% of cases, non of the "actually" relevant faker-mock-code never runs as it simulates the (rare) error case 50% of the times. And as you say, I can easily mock the error case with data: null, and an error code manually.

What I'd like to get from orval is the exact opposite, all the mock data that fits the api spec - exemption being the data wrapper element being null :-)

[soartec-lab]

@snext-twilde Thank you for your explanation. So, could you please tell me with actual code what kind of output you expect for what kind of input?

[snext-twilde]

sure, that would be the spec:

...
    Pet:
      type: object
      required:
        - id
        - name
      properties:
        id:
          type: integer
          format: bigint
        name:
          type: string
          nullable: true

    APIResponse:
      type: object
      required:
        - data
      properties:
        data:
          $ref: '#/components/schemas/Pet'
          nullable: true  # technically nullable: null may be returned on error
...

and instead of getting msw mocks with

... data: faker.helpers.arrayElement([<...>, null]). ...

I'd like to override this nullable just for the mock generation so I get:

... data: faker.helpers.arrayElement([<...>]). ...

Perhaps we could set a flag like

x-mock-non-nullable: true  # instructs Orval to generate mock data that is not null

or make it a config property in override.mock. ?

[soartec-lab]

@snext-twilde Ah, even if nullable: true, you don't want to include null in the generated mock response pattern, right? Is the reason why you set nullable: true in spite of the situation, as you told me before, that data will be null in case of an API error?

[snext-twilde]

@soartec-lab yep, exactly.

Although our discussion already shows that this is a little hard to wrap ones head around, probably not elegant enough.

From a use case view, we could say - nullable or not - we would like to customize the behavior of faker within msw mocks. Perhaps more straightforward....

The existing mechanism of mapping a property to a faker call is already close, we could perhaps expose slightly more of the underlying mechanism in the override config... along the lines of:

...mock...something... override: {
  nullableProperty: (nestedData) => faker.helpers.arrayElement([nestedData, null]), // this is what orval currently does
}

then I could override:

    data: (nestedData) => nestedData,  // get rid of the 50% mock cases carrying no data

What do you think?

[AllieJonsson]

I would say adding the possibility to remove nullable from specific endpoints or models would be great. Even just being able to disable the generation of the arrayElement([model, null]) to just be model instead would be a good first step. Ive also encountered this scenario in my projects where i would like to just not handle null values and only test scenarios where actual data is present

[soartec-lab]

@snext-twilde @AllieJonsson

Well, how about making the default mock function ​​individually exportable as shown below?

Before:

export const getListPetsResponseLabradoodleMock = (
  overrideResponse: Partial<Labradoodle> = {},
): Labradoodle => (
  faker.helpers.arrayElement(
    [
      {
        ...{
          cuteness: faker.number.int({ min: undefined, max: undefined }),
          breed: faker.helpers.arrayElement(['Labradoodle'] as const),
        },
        ...overrideResponse
      },
      null
    ]
  )
);

After:

export const getListPetsResponseLabradoodleDefaultMock = (
  overrideResponse: Partial<Labradoodle> = {},
): Labradoodle => (
  {
    cuteness: faker.number.int({ min: undefined, max: undefined }),
    breed: faker.helpers.arrayElement(['Labradoodle'] as const),
    ...overrideResponse
  }
);

export const getListPetsResponseLabradoodleMock = (
  overrideResponse: Partial<Labradoodle> = {},
): Labradoodle => (
  faker.helpers.arrayElement(
    [
      { 
        ...getListPetsResponseLabradoodleDefaultMock(),
        ...overrideResponse
      },
      null
    ]
  )
});

[AllieJonsson]

@soartec-lab I think Ive tried this before but I think it has broken along the way. If I recall correctly there is something like splitMockFunctions in the code, which should generate the exact code you wrote. I can take a look at it later today and see if I can figure out why its not working. I believe it broke after the changes I made in combine.ts