fix: support additional properties without xDictionaryKey #52

RicherStAmand · 2024-04-18T13:24:42Z

SamplesMarketplace swagger have this value :
https://api3-dev.landr.com/samplesmarketplace/swagger/v1/swagger.json#/

 "FilterCounters": {
        "type": "object",
        "additionalProperties": false,
        "properties": {
          "sampleTypeFilters": {
            "type": "object",
            "nullable": true,
            "x-dictionaryKey": {
              "$ref": "#/components/schemas/SampleType"
            },
            "additionalProperties": {
              "type": "integer",
              "format": "int64"
            }
          },
          "instrumentFilters": {
            "type": "object",
            "nullable": true,
            "additionalProperties": {
              "type": "integer",
              "format": "int64"
            }
          },
          "genreFilters": {
            "type": "object",
            "nullable": true,
            "additionalProperties": {
              "type": "integer",
              "format": "int64"
            }
          },
          "sfxFilters": {
            "type": "object",
            "nullable": true,
            "additionalProperties": {
              "type": "integer",
              "format": "int64"
            }
          },
          "tagFilters": {
            "type": "object",
            "nullable": true,
            "additionalProperties": {
              "type": "integer",
              "format": "int64"
            }
          }
        }
      },

that got type as is :

export interface FilterCounters {
    sampleTypeFilters: {
        [key in SampleType]: number;
    };
}

where I was expecting this :

export interface FilterCounters {
    sampleTypeFilters: {
        [key in SampleType]: number;
    },
    instrumentFilters: {[key: string]: number;},
    genreFilters: {[key: string]: number;},
    sfxFilters: {[key: string]: number;},
    tagFilters: {[key: string]: number;}
}

I found that we were not properly handling value with additionalProperties but without a xDictionaryKey.

Part 1 - This PR
Part 2 - #53
Part 3 - #54

SimonDrouin · 2024-04-18T13:49:16Z

src/helpers/getResultStringForAdditionalPropertiesType.ts

+            if (additionalProperties.items && additionalProperties.items[SwaggerProps.$ref]) {
+                res = `${parseRefType(additionalProperties.items[SwaggerProps.$ref].split('/'))}[]`;
+            } else {
+                res = `"// TODO: Something is wrong"`;


I wouldnt recommend to put TODOs in code even if its for logging purpose

I agree, but in this PR I focused on adding the support for additional properties type without dictionnary, without changing existing patterns. - This part of the code is in fact duplicated from the one that handle the additionalProperties that have dictionnary ref.

I have upcoming PRs to improve on this.

But at the end of the day, we will still want to write in the result file.
Something more like this ?
// Error: DataTypes.Array without items or items ref is not supported

Sure! Its more helpful

ardeois

LGTM
@nemrosim can you check as well, as you are the most familiar with this repo

nemrosim

LGTM, few comments

nemrosim · 2024-04-22T14:39:34Z

tests/getDescription.test.ts

+describe(`getDescription`, () => {
+    scenarios.forEach(config => {
+        it(`should return ${config.expected} when props is ${config.props}`, async () => {
+            const result = getDescription(config.props);
+
+            expect(result).toBe(config.expected);
+        });
+    });
+});


You could use it.each instead of scenarios. And why test is async?

it.each([['case 1',{input: output}],['case 2', {input, output}]])('%s ....', (_, props) => <do something>);

nemrosim · 2024-04-22T14:40:45Z

tests/getResultStringForAdditionalPropertiesType.test.ts

+describe(`getResultStringForAdditionalPropertiesType`, () => {
+    scenarios.forEach(config => {
+        it(`should return expected result when props is ${config.scenarioName}`, async () => {
+            const result = getResultStringForAdditionalPropertiesType(config.props);
+
+            expect(result).toBe(config.expected);
+        });
+    });
+});


Same here, async not needed too, I think.

nemrosim · 2024-04-22T14:44:06Z

src/helpers/getResultStringForAdditionalPropertiesType.ts

+    description,
+    propertyName,
+}: {
+    additionalProperties: any;


Why any? I see that you are testing additionalProperties with known props. (I understand that probably there could be some changes in the future, but at least we can define known props at the moment)

additionalProperties is returned by getSchemaProperties, which type it as additionalProperties: any
We don't have any type guard or logic in typesConverter to known the types of additionalProperties by the time we pass it to getResultStringForAdditionalPropertiesType.

of course, that could be improve, but in this PR I wanted to have a minimal number of change, just introducing the new helpers needed to resolve the issue I add with the samples marketplace types.

* fix: move helpers in individual files, improve typing * fix: replace 4 space by a tab characters, update unit test * fix: helpers for undefined type, ref and oneOf * fix: typing * fix: remove change that impact snapshot or expected result of unit test * fix: adjust props * fix: adding coverage (#54) * fix: add coverage * fix: update coverage * fix: improve error message * fix: improve error message * fix: convert space into tab charaters and use return character * fix: commit yarn.lock * fix: update test * fix: remove describe, remove test name

* fix: generate mock for additional properties without dictionnary * fix: update error message * fix: update mock generator and coverage

…c one

fix: support additional properties without xDictionaryKey

117f43f

RicherStAmand self-assigned this Apr 18, 2024

RicherStAmand requested review from MPeloquin, ardeois and pkiler April 18, 2024 13:25

ardeois requested a review from nemrosim April 18, 2024 13:32

RicherStAmand requested review from MathieuMathurin, SimonDrouin and jeremidallaire-landr April 18, 2024 13:45

SimonDrouin reviewed Apr 18, 2024

View reviewed changes

SimonDrouin approved these changes Apr 18, 2024

View reviewed changes

ardeois approved these changes Apr 18, 2024

View reviewed changes

RicherStAmand mentioned this pull request Apr 18, 2024

fix: move helpers in individual files, improve typing #53

Merged

pkiler approved these changes Apr 18, 2024

View reviewed changes

jeremidallaire-landr approved these changes Apr 18, 2024

View reviewed changes

nemrosim approved these changes Apr 22, 2024

View reviewed changes

RicherStAmand added 4 commits April 22, 2024 16:37

fix: update test

10feef0

fix: update test

1d12acc

fix: replace tab characters by 4 spaces

92438f9

fix: remove change that affect test result

1fe2ec6

RicherStAmand mentioned this pull request Apr 23, 2024

fix: adding coverage #54

Merged

RicherStAmand added bug Something isn't working Part 1 and removed bug Something isn't working labels Apr 23, 2024

RicherStAmand mentioned this pull request Apr 23, 2024

fix: generate mock for object without dictionary #55

Merged

RicherStAmand added 3 commits April 24, 2024 10:54

fix: generate mock for object without dictionary (#55)

595780a

* fix: generate mock for additional properties without dictionnary * fix: update error message * fix: update mock generator and coverage

fix: mock have to define for each properties even if its a problemati…

5b53c50

…c one

RicherStAmand merged commit c188273 into master Apr 24, 2024
1 check passed

RicherStAmand deleted the fix/SP-10428-generate-type-for-object-without-dictionnary branch April 24, 2024 18:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support additional properties without xDictionaryKey #52

fix: support additional properties without xDictionaryKey #52

RicherStAmand commented Apr 18, 2024 •

edited

SimonDrouin Apr 18, 2024

RicherStAmand Apr 18, 2024 •

edited

RicherStAmand Apr 18, 2024

SimonDrouin Apr 18, 2024

ardeois left a comment

nemrosim left a comment

nemrosim Apr 22, 2024

RicherStAmand Apr 22, 2024

nemrosim Apr 22, 2024

RicherStAmand Apr 22, 2024

nemrosim Apr 22, 2024

RicherStAmand Apr 22, 2024

RicherStAmand Apr 22, 2024

fix: support additional properties without xDictionaryKey #52

fix: support additional properties without xDictionaryKey #52

Conversation

RicherStAmand commented Apr 18, 2024 • edited

Choose a reason for hiding this comment

RicherStAmand Apr 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardeois left a comment

Choose a reason for hiding this comment

nemrosim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RicherStAmand commented Apr 18, 2024 •

edited

RicherStAmand Apr 18, 2024 •

edited