Skip to content

fix: Proper handling of clashing types case#109

Closed
candiduslynx wants to merge 8 commits intoinvopop:mainfrom
cloudquery:fix/duplicate-type-names
Closed

fix: Proper handling of clashing types case#109
candiduslynx wants to merge 8 commits intoinvopop:mainfrom
cloudquery:fix/duplicate-type-names

Conversation

@candiduslynx
Copy link
Copy Markdown
Contributor

@candiduslynx candiduslynx commented Oct 6, 2023 •
edited
Loading

Instead of #107

This PR aims to properly reference situations when different types (either shadowed ones or originating different packages, but having the same name) are referenced incorrectly (specifically, only the 1st entry is properly handled, other encounters of the same type name blindly reference the 1st definition, even if the types aren't the same).

To achieve this, the following changes were made:

  1. An internal _type field was added to jsonschema.Schema struct
  2. When adding new definition to the jsonschema.Schema.Definitions map, original reflect.Type value is stored in _type field
  3. When considering a definition to be referenced, no only the type name is taken into account, but also:
    1. package name
    2. whether the types can be assigned to each other
  4. If an entry isn't found by the name originally suggested (either inferred from type name, or from jsonschema.Reflector.Namer result), the code will try to locate a proper entry by adding -idx (e.g., -1, -2, etc.) suffix to the name value

@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Oct 6, 2023

@samlown I propose this short change instead of an extra option from #107 (it will allow to reference all unique types still, but no JSON pointers logic is required so no need to rely on the validation implementation to support that).

This was referenced Oct 6, 2023
Closed
Merged
Merged
kodiakhq bot pushed a commit to cloudquery/codegen that referenced this pull request Oct 6, 2023
@candiduslynx
fix: Fix type name clashing & nullability issues (#39)
6940313 
Blocked by:
* invopop/jsonschema#109
* invopop/jsonschema#110
kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Oct 6, 2023
@candiduslynx
feat: Introduce spec JSON schema (#14296)
c35f473 
Closes #14027

Blocked by:
* cloudquery/codegen#39
* invopop/jsonschema#109 – merged to `cloudquery/jsonschema@cqmain`
* invopop/jsonschema#110 – merged to `cloudquery/jsonschema@cqmain`

I propose reviewing the annotations along with tests, as the JSON schemas generated are just too long to grasp visually.
@candiduslynx candiduslynx force-pushed the fix/duplicate-type-names branch from 09ed1ca to 54cbbaf Compare October 7, 2023 19:13
@candiduslynx candiduslynx force-pushed the fix/duplicate-type-names branch from e987e0a to f11a450 Compare October 8, 2023 07:11
@candiduslynx candiduslynx force-pushed the fix/duplicate-type-names branch 3 times, most recently from 22c1413 to d62ac43 Compare October 8, 2023 08:25
@candiduslynx candiduslynx force-pushed the fix/duplicate-type-names branch from d62ac43 to ca3c866 Compare October 8, 2023 09:29
candiduslynx
candiduslynx commented Oct 8, 2023
View reviewed changes
reflect.go
return false
}

return a.AssignableTo(b) && b.AssignableTo(a)
Copy link
Copy Markdown
Contributor Author

@candiduslynx candiduslynx Oct 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is required for shadowing case, see TestShadowedClashingTypes

@candiduslynx candiduslynx mentioned this pull request Oct 9, 2023
Closed
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Oct 9, 2023 •
edited
Loading

@samlown I hope you're doing well in these uneasy days.

I've implemented the changes in #109 & #110 & tested them via a fork at github.com/cloudquery/jsonschema@cqmain.

The results of using the updated code along with NullableFromType: true, RequiredFromJSONSchemaTags: true is something I'm quite satisfied with.
You can find an example in schema.json file. It includes both the nullable properties based on the field type & the clashing types (you can easily find them searching for -1" suffix)).

Could you please give an estimate when these PRs (#109 & #110) could be merged? It'll be far easier to pin an upstream version instead of maintaining a fork for these 2 changes.

hydratim pushed a commit to hydratim/cloudquery that referenced this pull request Oct 20, 2023
@candiduslynx @hydratim
feat: Introduce spec JSON schema (cloudquery#14296)
8c98b86 
Closes cloudquery#14027

Blocked by:
* cloudquery/codegen#39
* invopop/jsonschema#109 – merged to `cloudquery/jsonschema@cqmain`
* invopop/jsonschema#110 – merged to `cloudquery/jsonschema@cqmain`

I propose reviewing the annotations along with tests, as the JSON schemas generated are just too long to grasp visually.
@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Nov 2, 2023

Hi @samlown!
Is there a chance to get this reviewed along with:

@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Nov 24, 2023

Hi @samlown! Is there a chance to get this reviewed along with:

Hi @samlown!
Could I have any ETA here?

@candiduslynx
Copy link
Copy Markdown
Contributor Author

candiduslynx commented Jul 5, 2024

@samlown I'm closing this PR for 2 reasons:

  1. I no longer have access to the fork the PR was opened from
  2. It seems to me that you just don't have the capacity to get this reviewed and merged

If you'll be available for review & merging please tag me here & I'll reopen from my own fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@candiduslynx