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

fix(restapi): Add cors headers when the request is returning errors #942

Merged
merged 2 commits into from
Oct 10, 2024

Conversation

2-towns
Copy link
Contributor

@2-towns 2-towns commented Oct 7, 2024

Currently, the REST endpoints don't return the CORS headers when they return an error.

One of the reasons is that Nim Presto has a weird behavior of adding the Access-Control-Allow-Origin header when a request is successful and omitting it when the request returns an error (see the method processRestRequest in nim-presto/presto/serverprivate.nim).

This PR fixes this behavior by adding the CORS headers whenever a response is returned. I was hesitant to create an OPTIONS endpoint for every request, as we did for the data upload, but it does not work with simple requests because no OPTIONS request is sent by the browser in that case.

Without the CORS headers returned when an endpoint returns an error, I am unable to display the error message to the user. Moreover, I am not able to know if the Marketplace is enabled or not (see codex-storage/codex-marketplace-ui#29).

Copy link
Member

@AuHau AuHau left a comment

Choose a reason for hiding this comment

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

Yeah it is pain that Presto does not handle this 😳 But yeah otherwise LGTM. Just fix the tests ;-)

@2-towns
Copy link
Contributor Author

2-towns commented Oct 8, 2024

Just fix the tests ;-)

I just ran the testAll and I have three tests failing:

[Suite] Marketplace contracts
  [FAILED] can be paid out at the end, specifying reward and collateral recipient

[Suite] On-Chain Market
  [FAILED] supports slot reservations full subscriptions
  [FAILED] pays rewards to reward recipient, collateral to host

But it doesn't seem related to my commit. Do you have specific tests that I should look into? @AuHau

@2-towns 2-towns added the Client See https://miro.com/app/board/uXjVNZ03E-c=/ for details label Oct 9, 2024
@2-towns 2-towns self-assigned this Oct 9, 2024
@2-towns
Copy link
Contributor Author

2-towns commented Oct 10, 2024

Just fix the tests ;-)

I just ran the testAll and I have three tests failing:

[Suite] Marketplace contracts
  [FAILED] can be paid out at the end, specifying reward and collateral recipient

[Suite] On-Chain Market
  [FAILED] supports slot reservations full subscriptions
  [FAILED] pays rewards to reward recipient, collateral to host

But it doesn't seem related to my commit. Do you have specific tests that I should look into? @AuHau

I just reran the tests and it worked

@2-towns 2-towns added this pull request to the merge queue Oct 10, 2024
Merged via the queue into master with commit 1fe3abf Oct 10, 2024
11 checks passed
@2-towns 2-towns deleted the fix/restapi/add-cors-headers-on-error branch October 10, 2024 09:38
veaceslavdoina pushed a commit that referenced this pull request Oct 21, 2024
…942)

* Add cors headers when the request is returning errors

* Prevent nim presto to send multiple cors headers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client See https://miro.com/app/board/uXjVNZ03E-c=/ for details
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants