Skip to content

Comments

Accept header parsing and upgrade bug#105

Open
r1mar wants to merge 67 commits intozackyang000:masterfrom
r1mar:master
Open

Accept header parsing and upgrade bug#105
r1mar wants to merge 67 commits intozackyang000:masterfrom
r1mar:master

Conversation

@r1mar
Copy link
Contributor

@r1mar r1mar commented Oct 12, 2022

Hello Zack,

ich have fixed one upgrade bug. The mongoose option reconnect tries doesn't exists any more. i deleted this from code.

I also improved the recognition of the Accept header. Multiple entries are now also supported and * placeholders.

xml and json are supported for the metadata. Only json is supported for other resources. If a mimetype is requested that is not supported, then 406 is returned.

Regards,
Richard

@zackyang000
Copy link
Owner

Hi r1mar,

  1. Thanks for fix option bug.

  2. Maybe we should have a default mimetype when the request doesn't contain a mimetype? Otherwise, you will not be able to use the browser to directly access resources. this is more commonly used in testing.

@r1mar
Copy link
Contributor Author

r1mar commented Oct 13, 2022

Hello zackyang000,

The protocol specifies xml as the default for the metadata. In principle, it is optional for data requests. However, since only json is implemented for, the choice is not large.

A test already existed for the metadata default. I moved this to a separate file and created another test for the default of the data requests.

In a nutshell. For the metadata the default is xml and for all other requests it is json.

If you don't fill the accept header, a response will always be sent. So it's not clear to me what you means.

Regards,
Richard

@r1mar
Copy link
Contributor Author

r1mar commented Oct 13, 2022

Hello zackyang000,

I have implemented the service document request. I also discovered a bug in the data requests when specifying the mimetype "/". I fixed the bug immediately. Maybe that's what you meant.

@zackyang000
Copy link
Owner

Hi r1mar,

Sorry, maybe I didn't make it clear. I mean the defaults are for resource requests, the defaults are request headers.

E.g:
Use this code to start the service.

var odata = require('node-odata');
var server = odata('mongodb://localhost/my-app');
server.resource('Book', {
    title: string,
    Price: Quantity
});
server.listen(3000);

forward:
RUN curl http://localhost:3000/books will get the resource content.

back:
If RUN curl http://localhost:3000/books -H 'Accept: application/json' gives the same result, which is fine.
But RUN curl http://localhost:3000/books will get a 406 result.

So, I mean, should we create a default header (eg Accept: application/json) when the request doesn't include it?

Finally, I found it seems to be a bug? see here.

@r1mar
Copy link
Contributor Author

r1mar commented Oct 14, 2022

Hello zackyang000,

getMediaType is only called when any accept header is given. If a user requests a format that is not supported, they must also get a 406. The default branch doesn't work because of this. Curl sends automatically the header accept: /. Support for asterix-header I implemented later here 2f27013 and one fix more here c44872f

You use for your test very old version of code. Can you test with last version of code? It should work.

@r1mar
Copy link
Contributor Author

r1mar commented Oct 18, 2023

Hello zackyang000,

I continued to extend of the OData implementation. The implementation is no longer dependent on MongoDB and can easily be used without a database or with another database. The metadata is no longer just used to publish the interface, but is also used for validation. There are completely new features such as batch requests, singletons and annotations. But we have a million breaking changes and the masochistic eslint validations are gone. ;-)

I am testing the implementation with the OpenUI5 framework client (https://openui5.hana.ondemand.com/). So far it's looking pretty good.

There is also a Yeoman Generator (https://www.npmjs.com/package/generator-men5) to generate apps with the men5 stack. For this I had to deploy a package myself. If you decide to accept this pull request, I will reference node-odata there.

Regards,
Richard

@zackyang000
Copy link
Owner

Hi @r1mar,

It would be great to receive your PR submission, but I tried running the unit test locally and it failed. Am I missing some configuration?

image

@r1mar
Copy link
Contributor Author

r1mar commented Nov 3, 2023

Hello zackyang000,

I got the error now too. I had to delete the lib folder to do this. I deleted the line. You don't need the plugin because a mapping between the OData and the internal layer can be specified. This means that database fields can have different names than the OData properties of the entities.

Regards,
Richard

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.

3 participants