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

object: fix acl and project team json marshalling #421

Merged
merged 2 commits into from Feb 23, 2021
Merged

Conversation

visualage
Copy link
Contributor

This partially address #420. The current ACL list does not comply with the response marshalling because the JSON field names are different than the storage.ACLRule field names. Created a type alias ACLRule from storage.ACLRule, and added custom Marshalling. Did same for storage.ProjectTeam.

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

I'm afraid this causes a breaking change for people using fake-gcs-server as a library :(

A safer approach would be to implement {Marshal,Unmarshal}JSON in the Object type.

@visualage
Copy link
Contributor Author

Done. Added custom marshaller to Object instead of changing the ACL type in Object.

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Looks good overall, let's unexport the types that are used just for marshaling. Also, can you add some tests?

fakestorage/object.go Outdated Show resolved Hide resolved
fakestorage/object.go Outdated Show resolved Hide resolved
@fsouza
Copy link
Owner

fsouza commented Feb 20, 2021

@tarsisazevedo does this PR fix the issue you were observing?

@tarsisazevedo
Copy link

I think so @fsouza. It will send all infos in the http response as the clients expect.

Copy link
Owner

@fsouza fsouza left a comment

Choose a reason for hiding this comment

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

Thank you very much! I think I'll add an example that uses the Java SDK later and that should take care of testing.

@fsouza fsouza merged commit 79ea211 into fsouza:main Feb 23, 2021
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.

None yet

3 participants