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

feat: adds odata metadata to entity type #74

Closed
wants to merge 6 commits into from

Conversation

snebjorn
Copy link
Contributor

fixes #60

lib/csdl2openapi.js Outdated Show resolved Hide resolved
@snebjorn snebjorn marked this pull request as ready for review March 31, 2020 21:58
@snebjorn
Copy link
Contributor Author

snebjorn commented Apr 1, 2020

I have no more outstanding issues/questions

@snebjorn
Copy link
Contributor Author

snebjorn commented Apr 9, 2020

@ralfhandl can we move forward with this?

Copy link
Contributor

@ralfhandl ralfhandl left a comment

Choose a reason for hiding this comment

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

Please adapt

lib/csdl2openapi.js Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
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
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
tools/V4-CSDL-to-OpenAPI.xsl Outdated Show resolved Hide resolved
@ralfhandl
Copy link
Contributor

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?

@snebjorn
Copy link
Contributor Author

snebjorn commented May 1, 2020

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
Comment on lines 4219 to 4200
"title": "Event (for update)",
"type": "object",
"properties": {
"@odata.type": {
"$ref": "#/components/schemas/type"
},
"ConfirmationCode": {
"type": "string",
"nullable": true
Copy link
Contributor Author

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 :(

Copy link
Contributor

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 😄

Copy link
Contributor Author

@snebjorn snebjorn May 19, 2020

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 :(

@@ -3301,6 +3301,9 @@
"title": "Location",
Copy link
Contributor Author

@snebjorn snebjorn May 3, 2020

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

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have autocrlf = false

Copy link
Contributor Author

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

@@ -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"
},
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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

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?

Copy link
Contributor Author

@snebjorn snebjorn May 25, 2020

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

@ralfhandl
Copy link
Contributor

Hi @snebjorn, is this PR still active/desired?

It seems to be almost ready for merging..

@snebjorn
Copy link
Contributor Author

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

@ralfhandl
Copy link
Contributor

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 .vscode settings, install the Prettier plugin, and reformat all source files before merging master into your branch.

Sorry for the hassle!

@snebjorn
Copy link
Contributor Author

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 .vscode settings, install the Prettier plugin, and reformat all source files before merging master into your branch.

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 :)
So this is a welcome change 👍

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 😎

@ralfhandl
Copy link
Contributor

Whiskey, not whisky? What's your recommendation?

My favorite is https://www.obanwhisky.com/whiskies/14-years-old.

@snebjorn
Copy link
Contributor Author

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/

@ralfhandl ralfhandl marked this pull request as draft January 12, 2023 12:06
@sterlp
Copy link
Contributor

sterlp commented Feb 24, 2023

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

@snebjorn
Copy link
Contributor Author

how far are you with the PR?

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.
You're welcome to continue the work here :)

@snebjorn
Copy link
Contributor Author

sorry, it's unlikely I'll finish this

@snebjorn snebjorn closed this Nov 15, 2023
@ralfhandl
Copy link
Contributor

Sorry for making things more complicated by refactoring while you were at it.

@snebjorn
Copy link
Contributor Author

that was not the reason. Priorities switched and I just never got back to this

@ralfhandl
Copy link
Contributor

That can happen to the best 😄

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.

SAP OData missing __metadata property
3 participants