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

[gen] dictionaries with referenced keys extension #3043

Merged
merged 1 commit into from
Aug 30, 2024

Conversation

hochgi
Copy link
Contributor

@hochgi hochgi commented Aug 21, 2024

  • works with or without alias flag properly.
  • validates non-string references.
  • tests for all UUID/aliased/non-string

fixes #3030
/claim #3030

@hochgi hochgi force-pushed the gen-issue-3030-dictionary-keys-with-ref branch from 8a2c68e to c49fdf9 Compare August 21, 2024 22:25
@hochgi hochgi force-pushed the gen-issue-3030-dictionary-keys-with-ref branch from c49fdf9 to c4ab55e Compare August 25, 2024 17:05
@hochgi hochgi requested a review from 987Nabil August 25, 2024 17:05
case (_, Left(_)) => left
case (Left(true), _) => right
case (Right(lSchema), Right(rSchema)) =>
val (leftKey, lAnnotations) = Object.extractKeySchemaFromAnnotations(lSchema)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure we should make it this complicated. Maybe it is okay we say we only support a certain sub set. Like only one key annotated schema?

Copy link
Contributor Author

@hochgi hochgi Aug 28, 2024

Choose a reason for hiding this comment

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

only one key means that of the 2 dictionaries: 1 is unconstrained, and 1 has a schema. in this case, code resorts to None, since we cannot have any schema.
The only applicable case is when we have 2 schemas.
I chose to implement "intersection" semantics when possible, over "union".

This is an important decision, which I'm not sure about.
What's your take? should we relax the key schema to be a union of both, or restrict to be the intersection of both?

Also, when does combine is called anyway?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think, id we have something that we can't resolve successfully, we should throw. At least something like not impl exception. Dropping it silently is hell for debugging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general I agree with the principle, not sure it applies here.

  • We're not dropping silently
  • it's an extension that is ignored by OpenAPI (anything that starts with x- is passed on transparently), thus I think we should avoid throwing as much as possible.

lets start with the obvious: if both are the same (either None, or identical Some(x)) - preserve.

The question is when they differ.
If only 1 is defined, i.e. combining 2 dictionaries, one restricted (e.g. UUID keys), the other is not.
So I assume the combined should not be constrained?

If both are defined, then some properties can be reconciled (e.g. min/max length or pattern), while others less so (format).
So I tried to do the best here.

What I lack, is the context of when combining actually happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@987Nabil IIUC, combine is called only when generating JsonSchema from zio Schema, i.e. when we define the endpoint in scala, and generate the schema from it.
If that's the case, I think that my decision to do an "intersection" was wrong, and I should do a "union".
I.e. max of maxLength (instead min of maxLength), | instead of lookahead assertions in pattern, etc'…

I'll change accordingly.

@987Nabil
Copy link
Contributor

Please also resolve the conflicts

@hochgi hochgi force-pushed the gen-issue-3030-dictionary-keys-with-ref branch from 164f56d to f4e6422 Compare August 28, 2024 08:40
@hochgi hochgi requested a review from 987Nabil August 28, 2024 08:42
* works with or without alias flag properly.
* validates non-string references.
* tests for: UUID/aliased/non-string
* revert Dictionary addition, and replace with an annotation
* handles writing the new extension when possible as well
* don't add extension if not needed
* no need for schema for plain string keys in the firstplace…
* add test for inlined (not ref) schema for dictionary keys
* adhere to CR comments, simplified code, and added comments
* union instead of intersection semantics when combining key schemas
@hochgi hochgi force-pushed the gen-issue-3030-dictionary-keys-with-ref branch from f4e6422 to 2e0fa22 Compare August 29, 2024 06:32
@hochgi
Copy link
Contributor Author

hochgi commented Aug 29, 2024

/claim #3030

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 39.43662% with 43 lines in your changes missing coverage. Please review.

Project coverage is 65.56%. Comparing base (e2f2cb5) to head (2e0fa22).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
...n/scala/zio/http/endpoint/openapi/JsonSchema.scala 39.43% 43 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3043      +/-   ##
==========================================
- Coverage   65.65%   65.56%   -0.09%     
==========================================
  Files         155      157       +2     
  Lines       10134    10246     +112     
  Branches     1899     1949      +50     
==========================================
+ Hits         6653     6718      +65     
- Misses       3481     3528      +47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@987Nabil 987Nabil merged commit ed4ce0f into zio:main Aug 30, 2024
93 of 94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[gen] support a schema for dictionary keys
4 participants