-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
OpenAPI: support sibling values for reference schemas #1020
base: master
Are you sure you want to change the base?
Conversation
- I.e. description, example, title and default - Fixes endpoints4s#888
Codecov Report
@@ Coverage Diff @@
## master #1020 +/- ##
==========================================
+ Coverage 75.75% 75.77% +0.01%
==========================================
Files 144 144
Lines 4764 4772 +8
Branches 57 59 +2
==========================================
+ Hits 3609 3616 +7
- Misses 1155 1156 +1
Continue to review full report at Codecov.
|
| "example": { | ||
| "id": "2cb38bf2-ecbf-401c-be37-9ec325ae91a0", | ||
| "title": "Essential Scala", | ||
| "author": { | ||
| "name": "Noel Welsh and Dave Gurnell" | ||
| }, | ||
| "isbnCodes": [ | ||
| | ||
| ], | ||
| "storage": { | ||
| "link": "https://underscore.io/books/essential-scala/", | ||
| "storageType": "Online" | ||
| } | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example is still here. I believe we should remove it from here. Probably, there is something wrong with the way we handle “named” schemas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're probably right that we should remove it, but I'm yet to find out how. With this added feature of silbing values, do we want to leave out examples etc. in the components/schemas
object altogether or is there still a use for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my it is fine to have examples in the components but here the issue is that the example is attached to the wrong model.
I believe the issue is that when we call "someSchema.named("foo").withExample(...)", the example is added to the schema foo although it should be added to schema that references foo instead (like you did, actually, but we also need to make sure that we don't also add the example to the referenced schema).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking more closely now, I see that you're right that it is attached to the wrong model. In fact, the behavior of the referenced schema example hasn't changed (yet). With this new code, the other models just have added schemas. So we'll either need to prevent the example from being added to components/schemas
or make sure that it contains the expected example.
In this case the first example is added to the generic Book
schema in this line: https://github.com/endpoints4s/endpoints4s/pull/1020/files#diff-8eee4ea78e0bdeb38dd981988cae59c64e82196da2d576610a4cfcc5f62b1629R77
As a a user I would expect this example to be added to each endpoint that uses the Book
schema, except if I override it with another example on a specific schema, like on this line: https://github.com/endpoints4s/endpoints4s/pull/1020/files#diff-8eee4ea78e0bdeb38dd981988cae59c64e82196da2d576610a4cfcc5f62b1629R99
Is this a correct assumption? If so, I can see two ways to achieve this in the documentation JSON: either leave examples out of components/schemas
entirely and do everything in the specific schemas, or make sure that the right examples are present in the referenced schemas. I can imagine that the former solution results in more verbose JSON, but is less complex to implement. I'll try some things and see what happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have some difficulty in understanding the purpose of original
and how it's set. If I search for usages of constructors with original
and withOriginal
I cannot find any, yet original
is set for all three Reference
schemas (two Book
and one Color
). Do you have an idea how original
is set via withExample
/withExampleJsonSchema
or other?
My understanding is the following. When we use To create a named schema, we just return the same schema with a Then, when we produce the OpenAPI “components”, we discard the schemas with duplicated names, so only one schema “wins”. Maybe a solution could be to handle “named” schema completely differently. Instead of having a field |
Thank you for your explanation, I understand it a lot better now!
Can your elaborate a bit more on the separate schema type? Right now there's a In the end we need to have a unique name for each referenced schema, and the right model should have the right schema reference. So I agree that we should do something different than |
Right now, the operation endpoints4s/openapi/openapi/src/main/scala/endpoints4s/openapi/JsonSchemas.scala Lines 401 to 408 in 24aa7a1
|
Just wanted to chime in and say I'm super excited to review this PR and intend to do so this weekend! |
I did some more digging into the code. The generic json schemas interpreter is new to me as well, so it took a while to familiarize myself with this part of the codebase.
Sorry if the questions don't make sense, I'm trying to wrap my head around the |
If possible, we should see if we can manage to not change the
At some point in the |
You're right, after some more tinkering I found out that the algebra doesn't need to be changed at all :)
Hmm, I didn't think of warning on detected duplicates as a possible outcome. I was actually working towards solving the duplicate by giving the new referenced schema a different name so that it would show up in the eventual OpenApi JSON. What do you think? I'm still connecting the dots, but slowly getting there. The references are built up in two places, here and here. The latter is the large Thanks for your help so far, btw! 🙏 |
I pushed a work-in-progress commit to demonstrate what I mean. The The code is far from ready, but I'm curious to know what you think of this approach. |
I am not sure, because it means that people manually calling In any case, users can always call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for only actually getting to this quite late. I've taken a look and I think this is an interesting idea - I'm going to go try out some things in a REPL now just to try to remember more about this problem.
At a high level, I remember thinking that we should only need to modify the DocumentedJsonSchema
by adding extra classes which only override examples/descriptions/etc. on the underlying schema (quite like RecordReference
). That way, we also wouldn't need to start generating schema names either.
Inserting extra names into the OpenAPI schema is a little bit unsavoury. From a theoretical perspective, it risks colliding with an actual user-provided name. More practically, some OpenAPI documentation generators (at least Swagger, but I think others too) list out all named schemas at the bottom of the page, and it would be nice not to see that list clouded with autogenerated stuff.
class RecordReference[A]( | ||
val name: String, | ||
referencedRecord: Record[A] | ||
) extends Record[A](referencedRecord.ujsonSchema, referencedRecord.docs.withName(name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting - I was expecting to see a subclass like this on DocumentedRecord
, not Record
.
Should it ever be possible to have a RecordReference
inside of a RecordReference
? Eg. RecordReference("foo", RecordReference("bar", RecordReference("baz", ...)))
? I see some effort here to avoid that case, but I can't tell yet if that is an invariant.
schema match { | ||
case s: RecordReference[A] => | ||
println(s"RECORDREFERENCE ${s.name}") | ||
s.withName(s.name + "$").withExample(exampleJson) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a clever idea, but I'm not sure it'll actually work unless the name mangling is made more complicated.
Consider the case where the same JsonSchema
is re-used twice to create two other schemas that just each have their examples changed. Since both calls for adding the example will use withExampleJsonSchema
, they'll both end up adding the same $
to the initial schema's name. We're now back where we started with a a collision in the schema name.
@agboom I resurrected and committed the old ideas I had a while ago on this topic into harpocrates@ef9aaa7. That also solves #888 (only for records, but the idea should work for the other nameable schemas) without needing to generate extra names. IIRC, I gave up mostly because I had run out of time to investigate. Maybe some of the ideas will be useful here... |
@harpocrates Thanks for pursuing this! Your solution looks a lot better than mine, so let's continue with that. |
@agboom let me know if I can help out with anything in particular, and thanks again for looking into this. 😄 |
@agboom are you still working on this? Would you mind if I picked it up? |
I reproduced the issue from #888 in an existing test
ReferencedSchemaTest.scala
by using thewithExample
method on some schemas. After some debugging I found out thatoriginal
contains the desired schema, so I came up with the solution to add theoriginal
value to the sibling fields. I'm quite new to the semantics of OpenAPI, so I'm not sure I got it right, but at least this test shows promising results. I also pasted the JSON in https://redocly.github.io/redoc and it rendered fine and with the examples in the right place.If the approach is right, maybe some additional tests with description, title and default are desired?