-
Notifications
You must be signed in to change notification settings - Fork 81
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
feat: adds odata metadata to entity type #74
Conversation
6bb870d
to
f609f64
Compare
I have no more outstanding issues/questions |
@ralfhandl can we move forward with 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.
Please adapt
also removed @odata.id as it's typically not in the response. Made @odata.type only show up when type had a basetype
they were accidentally committed
Good that the @id is gone, and the @type is there only for derived types. @etag actually is only there if the entity set uses optimistic concurrency control, in which case there also is an ETag response header for GET and an If-Match request header for POST, PATCH and DELETE. Just having @etag is incomplete and thus confusing. Maybe split the @etag change off into a separate pull request? |
It's only the etag I'm interested in, so I'd prefer it was there. But if we can get this merged quickly I can do an etag PR |
# Conflicts: # tools/V4-CSDL-to-OpenAPI.xsl # tools/tests/Northwind-key-as-segment.openapi3.json # tools/tests/Northwind-key-as-segment.swagger.json # tools/tests/TripPin.openapi3.json # tools/tests/TripPin.swagger.json # tools/tests/annotations.openapi3.json # tools/tests/annotations.swagger.json # tools/tests/authorization.openapi3.json # tools/tests/authorization.swagger.json # tools/tests/containment.openapi3.json # tools/tests/containment.swagger.json # tools/tests/csdl-16.1.openapi3.json # tools/tests/csdl-16.1.swagger.json # tools/tests/descriptions.openapi3.json # tools/tests/descriptions.swagger.json # tools/tests/odata-rw-v2.openapi3.json # tools/tests/odata-rw-v2.swagger.json # tools/tests/odata-rw-v3.openapi3.json # tools/tests/odata-rw-v3.swagger.json
examples/TripPin.openapi3.json
Outdated
"title": "Event (for update)", | ||
"type": "object", | ||
"properties": { | ||
"@odata.type": { | ||
"$ref": "#/components/schemas/type" | ||
}, | ||
"ConfirmationCode": { | ||
"type": "string", | ||
"nullable": true |
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.
@ralfhandl I'm having an odd issue with the test claiming stuff isn't there when it is
1) Examples
TripPin:
AssertionError [ERR_ASSERTION]: OpenAPI document
+ expected - actual
"type": "object"
}
"Microsoft.OData.SampleService.Models.TripPin.Event-update": {
"properties": {
+ "@odata.type": {
+ "$ref": "#/components/schemas/type"
+ }
"ConfirmationCode": {
"nullable": true
"type": "string"
}
as you can see in the diff, it's quite clearly there. No idea what going on :(
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 unit test npm run test
compares the current in-memory output with the "expected" output from the examples
folder.
If you are satisified with the diff, you can run npm run build
to refresh the "expected" output. Use with care 😄
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.
part of the problem here was the + expected - actual
output not having enough context to identify where the issue was.
I ended up running npm test > report
and counting how many lines it wrote to identify if I removed or added the @odata.type
in the correct place.
It wasn't fun :(
examples/TripPin.openapi3.json
Outdated
@@ -3301,6 +3301,9 @@ | |||
"title": "Location", |
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.
@ralfhandl Also need a bit of help here. When I run test.cmd
I get this error. It makes no sense to me.
odata-rw-v3
lexical error: invalid char in json text.
{"swagger":"2.0","info":{"titl
(right here) ------^
lexical error: invalid char in json text.
{"openapi":"3.0.0","info":{"ti
(right here) ------^
TripPin
lexical error: invalid char in json text.
{"swagger":"2.0","info":{"titl
(right here) ------^
lexical error: invalid char in json text.
{"openapi":"3.0.0","info":{"ti
(right here) ------^
As a result the json files are empty
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.
@ralfhandl do you have any clue why this is happening?
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 only cause I can think of is that test.cmd
only works if called from within the tools
folder.
Note that the XSLT and the JS versions are not in sync and have their own set of test inputs and expected outputs.
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 have a suspisseion this has something to do with CRLF and LF line endings.
I changed autocrlf = true
to autocrlf = input
a while ago and this problem started happening when I merged master in.
Also the CI doesn't seem to have this problem.
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 have autocrlf = false
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.
FML..... it was caused by a missing "
in a xsl:text
block
262ca65
to
cffcaa1
Compare
@@ -3061,6 +3061,9 @@ | |||
], | |||
"description": "The number of entities in the collection. Available when using the [$count](http://docs.oasis-open.org/odata/odata/v4.01/odata-v4.01-part1-protocol.html#sec_SystemQueryOptioncount) query option." | |||
}, | |||
"type": { | |||
"type": "string" | |||
}, |
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.
Probably better to not have this „reuse schema“, or only have it when it isn’t needed.
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 found this function deleteUnreferencedSchemas
which has the desired effect. But it also removes a lot of other unused schemas causing a bunch of tests to fail. Fixable though.
But I assume deleteUnreferencedSchemas
wasn't used for a reason?
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.
Reason is that there can be CSDL files with schemas for reusable types and no entity container. The types are only referenced from other files, not from within the same file.
Consequently "delete unreferenced schemas" is not on by default.
Easiest fix here is not to define a reuse schema and generate @type
directly with type: "string"
.
@@ -12,6 +12,9 @@ | |||
"Supported.Annotations.SinglePartKey": { | |||
"type": "object", | |||
"properties": { | |||
"__metadata": { | |||
"$ref": "#/definitions/metadata" |
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.
Don’t see this in definitions: lost?
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.
Good find. This is strange.
I'm running test.cmd
to update the json files. I double checked and It doesn't generate the metadata definition.
But it doesn't generate any of the reusable schemas. Hmmm the paths
property of the OAS is empty. Perhaps that's part of the issue
Hi @snebjorn, is this PR still active/desired? It seems to be almost ready for merging.. |
Hi @ralfhandl, sorry. It's still desired. I've just been lazy. Lets hope for really bad weather - then I'll get around to doing it :D |
Hi @snebjorn, I hope the weather is getting worse daily 😄 . I added prettier and eslint, so the code base is now formatted differently. Best you first copy over the Sorry for the hassle! |
No problem at all. I was thinking of doing that at some point as well. I had to do ctrl+shift+p and save without formatting cause I had format on save on :) Status on the PR. I tried to fix it a few weeks ago. But man this is hard getting back into. I'll need to get some more top shelf whiskey 😎 |
Whiskey, not whisky? What's your recommendation? My favorite is https://www.obanwhisky.com/whiskies/14-years-old. |
Oh yes Whisky, I thought it looked a little funny. I've had some good times with https://www.aberlour.com/en/product/abunadh-scotch-whisky/ |
@snebjorn how far are you with the PR? I might need it too, as our gateway will most likely otherwise fail the schema validation on the response path. As the fields are where without beeing mentioned in the schema. |
This PR is quite out of date now. I'm probably not going to finish this any time soon. My workarounds have been working, so getting this fixed is very low on my todo list. |
sorry, it's unlikely I'll finish this |
Sorry for making things more complicated by refactoring while you were at it. |
that was not the reason. Priorities switched and I just never got back to this |
That can happen to the best 😄 |
fixes #60