Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Proper handling of clashing types case #109

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

candiduslynx
Copy link
Contributor

@candiduslynx candiduslynx commented Oct 6, 2023

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
Contributor Author

@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).

kodiakhq bot pushed a commit to cloudquery/cloudquery that referenced this pull request Oct 6, 2023
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 3 times, most recently from 22c1413 to d62ac43 Compare October 8, 2023 08:25
return false
}

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

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
Copy link
Contributor Author

candiduslynx commented Oct 9, 2023

@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
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
Contributor Author

@candiduslynx
Copy link
Contributor Author

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

Hi @samlown!
Could I have any ETA here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant