-
Notifications
You must be signed in to change notification settings - Fork 223
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
Sample services don't support CORS, which makes JS clients hard to consume #29
Comments
I wanted to try and make an OData v4 CRUD client to Trippin using AngularJS but it is simply blocked without CORS support. Any suggestions on how to enable it? |
For those who come looking, I have a workaround that seems to be working with the Trippin sample. I used the enable-cors site http://enable-cors.org/server_iis7.html |
I spoke too soon, the server does not reply to the OPTIONS preflight approval request fired from the browser and the workaround does not actually work. :( |
Hi! I want to know if this will get resolved in the future. Cheers |
Any plans to fix that? Access-Control-Request-Method should allow PATCH, PUT, POST and DELETE. |
Hi All, |
Would be nice if sample services can finally support CORS. I'd like to avoid using proxy servers, especially when it comes to quick demos. |
same for me |
Does someone take care about this issue? Also SAP uses this sample service in their OData V4 example and I get the "Status Code: 404 Not Found" back when removing the mockserver. How can I help you @biaol-odata @markdstafford @mikepizzo @xuzhg to fix this? |
This comment has been minimized.
This comment has been minimized.
@mikepizzo Awesome, but this issue shouldn't be closed yet as the original issue was referring to V2 and V3 Northwind services. Or does #113 cover them too? |
Re-opening as V2/V3 not yet fixed, and we have not yet deployed the fix to the hosted services. |
Is there any word on this being deployed to the service yet? |
So the day has come. The popular public proxy server
@mikepizzo How can we help to fix this issue? Would it be possible to deploy at least the V4 fix to the hosted sample services? |
Sorry; I didn't realize this was still an issue for folks. The service has supported returning CORs headers on GET/POST/PUT/PATCH requests, but didn't previously support OPTIONs for CORs preflight (OPTIONs requests returned 404 not found). TrippinRESTier sample service now supports OPTIONs for CORs preflight (basically, and request to OPTIONS will return the CORs headers and 200 OK. @boghyon -- let me know if you're still running into issues with Trippin. |
@mikepizzo Thank you for the reply. Unfortunately, it still fails:
|
From the error message it looks like we are setting the Access-Control-Allow-Origin header twice. Indeed, as I look at the code, it looks like we tried to support it through a CORS add-in as well as specifically set the header (just to be sure) which may be leading to having it set in two places. I haven't been able to validate this in Fiddler (which I think messes with my Origin request header), and I don't see the error in embed.plnkr.co (I just get "no data") but I've uploaded a possible fix to a staging service. Can you try https://services.odata.org/Trippin_Staging/(S(iw1anra4xygjyssbeef0yeyy))/$metadata and let me know if it fixes the issue? If so, I'll deploy the same fix to production (which is simply removing the explicit setting of the Access-Control-Allow-Origin header and relying on the CORS library.) |
I updated the service URL but unfortunately the error remains.
|
@boghyon -- Sorry, I missed copying over a config file with the change for the redundant Allow-Origin header. Please try https://services.odata.org/Trippin_Staging/(S(iw1anra4xygjyssbeef0yeyy))/ one more time, and let me know what you find. Unfortunately I don't see an error when I go to https://embed.plnkr.co/IAM5TBfKWaTW8vbg?show=manifest.json,preview&deferRun, I just get "No Data". |
@mikepizzo I do get Error log from the browser console:
According to the OData V4.0 spec 8.1.5:
Same spec can be found in the V4.0.1 section 8.1.5. Maybe I'm misinterpreting the specification or overlooking something. Nevertheless, the client (UI5) does seem to expect And according to https://github.com/SAP/openui5/blob/3375063aa9fc6344440978f2f7cb5b93a249da31/src/sap.ui.core/src/sap/ui/model/odata/v4/lib/_Requestor.js#L666-L668, the Edit: I just noticed that there is an |
@mikepizzo Now the question is, does the service have to add the |
I generally try to make clients a little lax so that they can work with a broader range of services, which means I would probably try and default the client to OData-Version: 4.0 if there was no response header, but that's just me. I know there are other equally valid design philosophies out there as well. We don't really talk about OPTIONS in the OData spec, but I would generally expect the same header rules apply as for the actual request, so I've added the OData-Version header to Trippin_Staging and confirmed that both the Access-Control-Allow-Origin and OData-Version headers are returned. I still get No data in the browser for the Open Ui5 app, but perhaps you'll have more luck? |
I agree. UI5 (SAP's client-side JS framework) is quite strict when it comes to OData V4 currently. I asked if they could at least ignore the missing headers in preflight responses and make assumptions if possible.
Unfortunately, the |
Request
Request headers
Response headers
(Tried to upload the HAR file for better analysis but GitHub doesn't allow it) Reproducible with https://embed.plnkr.co/IAM5TBfKWaTW8vbg?show=manifest.json,preview&deferRun (Headers copied from Chromium's Network tab devtool). |
AFAIK, the But more importantly, after discussing with the V4 maintainers from UI5 (SAP/openui5#3150), the reason why the browser is unable to access the
And JS clients (e.g. UI5) depend on the API To sum it up:
Could you add |
@mikepizzo Is there a chance to continue with this issue? Adding Would be nice if this issue could be reopened as it's not fully resolved. |
Yes -- hope to get to this after the long week-end. |
@boghyon -- sorry for the delay; the changes were trivial, but some system changes to the production environment made it difficult for me to push the changes. Please verify that https://services.odata.org/Trippin_Staging/(S(iw1anra4xygjyssbeef0yeyy))/ works correctly (i.e., that OPTIONS now returns the Allow-Control-Expose-Headers:* as well as the OData-Version header). Assuming this addresses the issue, I will push the changes from staging to production. Thanks for your patience --- -Mike |
Hi @mikepizzo , I've created a REST Client script at odata.org-Trippin.http and included the results. Please check if that is what you expect. Best regards |
@mikepizzo Sorry, I just realized I that I wrote @gregorwolf Thanks for the script! Didn't know about the extension. Request from the client with the following headers### Test Trippin_Staging
OPTIONS https://services.odata.org/Trippin_Staging/(S(iw1anra4xygjyssbeef0yeyy))/People?$select=FirstName,UserName&$skip=0&$top=100
Accept: */*
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9
Access-Control-Request-Headers: content-type,odata-maxversion,odata-version,x-csrf-token
Access-Control-Request-Method: GET
Cache-Control: no-cache
Connection: keep-alive
Host: services.odata.org
Origin: https://run.plnkr.co
Pragma: no-cache
Referer: https://run.plnkr.co/
Sec-Fetch-Dest: empty
Sec-Fetch-Mode: cors
Sec-Fetch-Site: cross-site
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/91.0.4472.101 Safari/537.36 Response headersHTTP/1.1 200 OK
Cache-Control: no-cache
Pragma: no-cache
Expires: -1
Server: Microsoft-IIS/10.0
Access-Control-Allow-Origin: *
Access-Control-Allow-Headers: content-type,odata-maxversion,odata-version,x-csrf-token
X-AspNet-Version: 4.0.30319
X-Powered-By: ASP.NET
Date: Tue, 15 Jun 2021 09:20:08 GMT
Content-Length: 0 Interestingly, neither |
Okay; this has been a strange trip.... If I enable CORS using the EnableCorsAttribute, then the appropriate CORs headers are automatically added to supported methods. Leveraging that just required that I add (and register) an OPTIONS controller method to handle he preflight requests. However, if CORS is enabled using the EnableCorsAttribute, and Access-Control-Request-Method: GET request header is specified, then:
So, I had to add a message handler that, for OPTIONS requests, adds the OData-Version and (if it's not already present) the Access-Control-Expose-Headers response header. I have uploaded the latest to staging -- you can try it at https://services.odata.org/Trippin_Staging/(S(iw1anra4xygjyssbeef0yeyy))/. Please let me know if this still has any issues; I think I now have hooks in all the right places in code to modify the behavior as required. |
I just tested basic CRUD, with and without $batch. So far everything seems to work well! |
Thanks for the confirmation -- glad we finally got it sorted! I've pushed to production, so I'll close this issue; please let me know if you encounter any further issues. |
@mikepizzo Would it be possible to enable CORS for other services as well? Especially the V2 Northwind is still widely used for demos and tutorials. |
@boghyon -- please go ahead and open a separate issue to track supporting CORS in the other services, and reference this one for the history/background. The V2 stuff is pretty old, and in a different repo that's not actively maintained, so I'm not sure when we'll have cycles to go back and update, but at least we'll be able to track the request. |
The original issue is here: OData/odataorg.github.io#66
Quoting the author:
The reporter was able to reproduce this problem against V2, V3 and V4 Northwind services. He hasn't tested TripPin services yet, but they may have the same problem.
The text was updated successfully, but these errors were encountered: