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

Added virtual URL style support and working tests. #41

Merged
merged 10 commits into from
Sep 22, 2023

Conversation

djcard
Copy link
Contributor

@djcard djcard commented Sep 20, 2023

There is a second style of S3 URL which the SDK didn't support yet. This should support it. Existing integration tests running and unit tests added as well.

@jclausen
Copy link
Contributor

All the tests are failing on this right now, so we will need to wait to review until they are passing. De-coupling the utils from inheritance is tricky because there are methods in the super scope that are used as first-class in the Util components. Those methods are also used in the S3 component.

If you wish to de-couple those, you will probably need to extract those methods to a separate Util component and then refactor both the SDK and the Utils to use that. Unfortunately, you can't use delegates because this module needs to work with Coldbox 6.

@jclausen
Copy link
Contributor

@michaelborn and I discussed moving those utils to a separate S3 module, since the ability to generate signatures is universal in authenticating against AWS APIs and we ended up having to commit a copy of the V4 util to source control, with the missing methods added in. Michael, do you remember what methods were the ones from AmazonS3.cfc that the v4 and v3 utilities used?

@djcard
Copy link
Contributor Author

djcard commented Sep 20, 2023

I probably shouldn't even have touched the Wirebox bindings, I just thought it was odd that they were both bound to the thing and that it was a cut / paste error but now that I think about it, that was faulty thinking, even on its face. I've reverted those back to their original values. My stuff only had to do with forming the URL and didn't touch any of the deeper aspects of the module.

@jclausen
Copy link
Contributor

One reason we have always used path-based URLs instead of virtual, in this SDK, is that, any bucket name with a . in it will have an invalid SSL certificate - which will then blow up on the http requests to AWS. We may want to make a note in the docs on that if we support it.

I can see this being handy with Cloudfront distributions, which proxy to the API. We would probably want to modify the code in the virtual conditional, though to check for the existence of amazonaws.com in the hostname, like we do with path-based. Then we could return signed URLs with the distribution domain ( e.g. https://cdn.mydomain.com/my/key.png )

Copy link
Contributor

@jclausen jclausen left a comment

Choose a reason for hiding this comment

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

A few changes I would like to see before we merge.

models/AmazonS3.cfc Outdated Show resolved Hide resolved
models/AmazonS3.cfc Outdated Show resolved Hide resolved
models/AmazonS3.cfc Outdated Show resolved Hide resolved
@jclausen jclausen merged commit 6d52a53 into coldbox-modules:development Sep 22, 2023
1 check passed
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