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

Add an OPTIONS response #189

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add an OPTIONS response #189

wants to merge 6 commits into from

Conversation

palewire
Copy link

@palewire palewire commented Jul 5, 2022

This proposal would add support OPTIONS request with a simple permissive response. My hope is that this will allow for integration with SSL systems.

For #179. cc @ngbrown

@oittaa
Copy link
Owner

oittaa commented Aug 3, 2022

Would you mind adding a test or few for the OPTIONS method?

@palewire
Copy link
Author

palewire commented Aug 3, 2022

Sure. Is there an example in the repository now you'd like me to emulate?

@palewire
Copy link
Author

palewire commented Aug 3, 2022

I believe I have added a test for you, @oittaa, but the workflow needs your permission to run.

@palewire
Copy link
Author

palewire commented Aug 9, 2022

I've made some changes to address issues in the tests and linter.

@silochad
Copy link

silochad commented Sep 4, 2022

This would be very helpful for our team as well, currently blocked on using this locally due to CORS issues.

@tbiq
Copy link

tbiq commented Sep 14, 2022

This would be very helpful for our team as well, currently blocked on using this locally due to CORS issues.

Funny seeing you here haha. Literally just ran into this issue with our local dev setup and this is where I came to try and figure it out.

More generally speaking, anything we can do to help speed up the integration of this feature into the library @oittaa?

Very appreciative of all the work you put into this library. Hard to believe that google doesn't support anything like this natively in their storage client library.

@palewire
Copy link
Author

I'd love to have somebody test my PR to see if it works for them.

@tbiq
Copy link

tbiq commented Sep 14, 2022

I'd love to have somebody test my PR to see if it works for them.

Will do! I'll let you know once I've tested things.

@tbiq
Copy link

tbiq commented Sep 15, 2022

@palewire not sure if your use case is related to CORS or not, but when I was testing this, I ran into the issue that even though the options were being set correctly (after the changes in my other comment), I was still running into issues because subsequent responses didn't use the Access-Control-Allow-Origin header. If you read the COR's documentation here you can learn more about this.

Not sure if we would classify this within the scope of this PR or as something separate (though the changes to options are required first to make this work).

As a quick and dirty fix, in my own local installation of the package, I added

if method == "GET": 
    response['Access-Control-Allow-Origin'] = "*"

Right before the return statement in the method handle of the Router class in server.py. This definitely isn't the right way to implement this in a generalizable approach, but it works for my use case.

One thought I had would be to

  1. Add a top level arg to the Server init method called enable_cors
  2. If this flag is enabled (False by default to be backwards compatible), we would add the header Access-Control-Allow-Origin: * to any outgoing response, regardless of the method (GET, POST, etc.) of the incoming request.

This could also be made more complex by enabling cors only for specific buckets, rather than for the emulator as a whole. But for my purposes this simple approach would suffice.

Curious to hear @oittaa 's perspective on what the right way to implement this would be.

@bam4564
Copy link

bam4564 commented Sep 30, 2022

@oittaa bumping this

@vmg-dev
Copy link

vmg-dev commented Nov 21, 2023

Is there a reason this hasn't been merged?

Copy link
Owner

@oittaa oittaa left a comment

Choose a reason for hiding this comment

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

lgtm

@oittaa
Copy link
Owner

oittaa commented Aug 3, 2024

I haven't had much time following this project, but recently I've updated it to support Python 3.12 and so on. Could you fix the remaining issues?

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.

6 participants