fix: allow to override only root mock

[karlismelderis-mckinsey]

Status

READY

Description

Allow to override only root of mocked data and type overrideResponse argument as Partial<Type> instead of any

I also regenerated all react hook samples and rerun format for all repo hence there are quite some unrelated diff

Fix #1335

I'll try to find time to do two more follow up PRs:

• move to yarn 4.x
  • it took me a while to get project working 😵‍💫
• have only one petstore.yaml for all samples
  • there is a lot of code duplication going on

[melloware]

I love your ideas of Yarn 4 and cleaning up the tests as I do find it quite overwhelming. Right now it looks like there is a test failure though?

[melloware]

Fixed tests

[karlismelderis-mckinsey]

I tried failing test in main and it also fails there

break doesn't feel related to my changes 🤔

[karlismelderis-mckinsey]

Feels like tests/specifications/polymorphic.yaml is generating impossible type

[melloware]

You could be right it might not be related to your change.

[karlismelderis-mckinsey]

I added a fix for polymorphic.yaml
unless you want to cherrypick in another PR

[soartec-lab]

Please check some comments. Also, there seem to be some conflicts, so please check those as well.

[soartec-lab]

Could you please definition in more detail why this description is no longer necessary?

[karlismelderis-mckinsey]

So in main generated code looks like this:
Notice multiple ...overrideResponse and overrideResponse is of type any

export const getGetPolymorphicResponseResponseMock = (
  overrideResponse: any = {},
): GetPolymorphicResponse200 =>
  faker.helpers.arrayElement([
    {
      key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
      type: faker.helpers.arrayElement([
        faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
        undefined,
      ]),
      value: faker.helpers.arrayElement([{}, undefined]),
      ...overrideResponse,
      ...overrideResponse,
    },
    {
      key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
      type: faker.helpers.arrayElement([
        faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
        undefined,
      ]),
      value: faker.helpers.arrayElement([{}, undefined]),
      ...overrideResponse,
      ...overrideResponse,
    },
  ]);

as this PR stoped overriding nested data in generated code same part looks like this:

export const getGetPolymorphicResponseResponseMock =
  (): GetPolymorphicResponse200 =>
    faker.helpers.arrayElement([
      {
        key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
        type: faker.helpers.arrayElement([
          faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
          undefined,
        ]),
        value: faker.helpers.arrayElement([{}, undefined]),
      },
      {
        key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
        type: faker.helpers.arrayElement([
          faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
          undefined,
        ]),
        value: faker.helpers.arrayElement([{}, undefined]),
      },
    ]);

as spread of any is gone TS starts to complain because GetPolymorphicResponse200 is impossible type

here is a link to TS playground
https://www.typescriptlang.org/play/?#code/C4TwDgpgBACghgJwgO2AFXBDkoF4qiQD2AZrIiupthANoDWEIpBmL8SqNNAugNwAoAQGMiyAM7BynKpBp4oAbwFQoAIQDyGgDIBRAIIA5AFxQA5Jp0HDZgDQqoAZTQAlAJKGA4qbPP3XuwEAXyg4cShRCWBBAQgADzAiBCkAS1QIBBI4YWgOSnllVUYQAH5TSQQ0gHNBVUIIMul86kxaqAA3OAAbAFcG00UoBiZy4ErkKp5TOGQQKCDBIKF6qAARCHEc5AATGeANZGh8PK5MKAAyJQdO3v6oACMiIi6IGcXBeMTk1hx1zZRdlwAO5EBQnWTQS6FDrdPqNCrVd7LM6eCDAGDPEAAWySYAAFilhC4NokJBAAEwABkpCj+W0B+0OUAAPmsNvS9mgQTFIpIoGIIKZUejMTiEPjCcTxKTxBTqQpoUURuYBYFVHVMD5LHojGr1Tc+qYxn17EsRGI+cAQUK0RiutjcQSiSSLXKafhFVBij4rUQ9RrID4-B5PP6YbcfDM5giJoEzbypFGDkcruqvcqzKr7Gn6lqtDqbNn9bDBQQECaHKoAHQ1xQhMKhWamoA

I don't know at which point things got broken because looking at original polymorphic.yaml in swagger editor
value key should be ignored from ParentType

Would make sense to do a followup PR rather than try to fix things in this PR

[soartec-lab]

Thank you for letting me know so politely.
In other words, the mock was originally broken, and by expanding overrideResponse, the error was suppressed and it worked, right?

[soartec-lab]

As you said, the actual failure occurred at a different time, but it is true that what was previously working will no longer work as a result of taking this action.
This will result in the bug being included in the next release unless you follow up after merging this PR.
It doesn't necessarily have to be the same PR, and I think it's better to separate them, but is it possible to follow up at the same time?

[AllieJonsson]

One possible solution could be to type the different possibilities, like this:

// GetPolymorphicResponse200 = DescendantOne | DescendantTwo
faker.helpers.arrayElement([
  {
    key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
    type: faker.helpers.arrayElement([
      faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
      undefined,
    ]),
    value: faker.helpers.arrayElement([{}, undefined]),
  } as DescendantOne,
  {
    key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
    type: faker.helpers.arrayElement([
      faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
      undefined,
    ]),
    value: faker.helpers.arrayElement([{}, undefined]),
  } as DescendantTwo,
]);

One improvement here could maybe also be to add the "overridden" type to the array as well? Something like

faker.helpers.arrayElement([
  {
    key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
    type: faker.helpers.arrayElement([
      faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
      undefined,
    ]),
    value: faker.helpers.arrayElement([{}, faker.datatype.boolean(), undefined]),
  } as DescendantOne,
  {
    key: faker.helpers.arrayElement([faker.word.sample(), undefined]),
    type: faker.helpers.arrayElement([
      faker.helpers.arrayElement(['BOOLEAN', 'STRING'] as const),
      undefined,
    ]),
    value: faker.helpers.arrayElement([{}, faker.word.sample(), undefined]),
  } as DescendantTwo,
]);

[karlismelderis-mckinsey]

feels like I opened can of worms 😅

Issue is not in faker code, it's GetPolymorphicResponse200 that's broken.
that type cannot be used even outside of faker.

I don't mind to investigate the case but let's consider this

generated types are broken already.
as no-one reported broken types probably it means no-one has such use case in their project.

hence I would argue that we're not actually breaking anyones code.

I'm on vacation next week so any update from my side will arrive with delay if I don't dig up anything today.

[karlismelderis-mckinsey]

So I traveled back to commit where polymorphic.yaml was added

faker code were not typed at that time and generated types behave as shown here

export const getGetPolymorphicResponseMock = () =>
  faker.helpers.arrayElement([
    {
      value: faker.helpers.arrayElement([{}, undefined]),
      key: faker.helpers.arrayElement([faker.random.word(), undefined]),
      type: faker.helpers.arrayElement([
        faker.helpers.arrayElement(['BOOLEAN', 'STRING']),
        undefined,
      ]),
    },
    {
      value: faker.helpers.arrayElement([{}, undefined]),
      key: faker.helpers.arrayElement([faker.random.word(), undefined]),
      type: faker.helpers.arrayElement([
        faker.helpers.arrayElement(['BOOLEAN', 'STRING']),
        undefined,
      ]),
    },
  ]);

I suspect Orval never really supported situation that was created in polymorphic.yaml

According to Official Swagger docs Orval is generating correct types and polymorphic.yaml is broken.

There is no hierarchy in allOf hence data should comply with all schemas thus I don't think ignoring value: Record from ParentType is a way forward.

[soartec-lab]

@karlismelderis-mckinsey

Thank you for investigating.
I understand that Polymorphic is not working and the types being generated are flawed, and the problem manifests itself not only in mocks. What I was concerned about was breaking a working state in an unintentional way.
However, even if we only take care of the mock with this PR, the fundamental problem will not be resolved and it is not essential, so we agree to treat it as a separate issue in favor of the original purpose of this PR.

[karlismelderis-mckinsey]

@soartec-lab generated types are not flawed.
Quite contrary, they show just what polymorphic.yaml intends to.

But polymorphic.yaml is flawed as a test example.
value key should not be part of ParentType schema

Hence if we agree to my change (removal of value from ParentType) I would consider test data fixed and case closed

[soartec-lab]

@karlismelderis-mckinsey

I see, I understood that there is no problem with the generated type, but the problem is with the OpenAPI definition. If this change causes problems for some users, they will need to modify their own OpenAPI definitions.

I was able to understand correctly. And I think that's correct.

[soartec-lab]

@karlismelderis-mckinsey
I commented, could you please check it?

[soartec-lab]

@karlismelderis-mckinsey

There is no problem with the source code so far, but since it conflicts with #1199, is it okay to wait a little longer for the response in #1199? And I would like to check the source code again and then merge this. Is it OK?

[melloware]

Yes that is ok

[soartec-lab]

@karlismelderis-mckinsey
Yes, I'll merge it. thank you for nice update!

[soartec-lab]

Hi @Maxim-Mazurok, I wanna merge this but you are in "request change" status. Could you please check this again?

I guess it was fixed

[Maxim-Mazurok]

Hi @Maxim-Mazurok, I wanna merge this but you are in "request change" status. Could you please check this again?

I had a very brief look and dismissed my review, unfortunately don't have a capacity for a full review now

[karlismelderis-mckinsey]

@karlismelderis-mckinsey Yes, I'll merge it. thank you for nice update!

Let's merge it before any new conflicts arrive 😉

[soartec-lab]

@karlismelderis-mckinsey
OK, I merged it. Let's look forward to the next release together!

[karlismelderis-mckinsey]

@karlismelderis-mckinsey OK, I merged it. Let's look forward to the next release together!

So I know how to plan our time, when do we expect to have next release?

[soartec-lab]

I think it will be soon.

@anymaniax

Do you have plan to next release soon?