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

Additional object response attributes #1524

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

manuteleco
Copy link
Contributor

@manuteleco manuteleco commented Mar 8, 2024

Some GCS client libraries (like this one), are quite strict when it comes to the set of attributes they expect to find in the response. If any of those attributes is missing, they fail to deserialize the response.

Here we introduce three new attributes in the Object response:

  • SelfLink
  • MediaLink
  • Metageneration

SelfLink cannot be tested using the Go GCS client library, as it is not included in the ObjectAttrs struct.

For the Metageneration we just hardcode the value "1", considering that we are not keeping track of it internally and "1" happens to be the correct value most of the times anyway.


Edited: I believe this PR fixes #758, which I only noticed after the fact.

Some GCS client libraries (like [this one][1]), are quite strict when it
comes to the set of attributes they expect to find in the response. If
any of those attributes is missing, they fail to deserialize the
response.

Here we introduce three new attributes in the Object response:
* `SelfLink`
* `MediaLink`
* `Metageneration`

`SelfLink` cannot be tested using the Go GCS client library, as it is
not included in the [`ObjectAttrs`][2] struct.

For the `Metageneration` we just hardcode the value `"1"`, considering
that we are not keeping track of it internally and `"1"` happens to be
the correct value most of the times anyway.

[1]: https://crates.io/crates/google-cloud-storage
[2]: https://pkg.go.dev/cloud.google.com/go/storage@v1.39.0#ObjectAttrs
@manuteleco
Copy link
Contributor Author

Note that this PR will conflict with #1520 around the definition and use of newProjectedObjectResponse. Once one of them is merged, I could rebase and resolve conflicts on the other.

In case that's helpful, here are the changes from all of my PRs from today combined into a single sequence of commits: manuteleco/fake-gcs-server@abe467c...0887d58.

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!

@@ -186,6 +186,14 @@ func checkObjectAttrs(testObj Object, attrs *storage.ObjectAttrs, t *testing.T)
t.Errorf("wrong MetaHeader returned\nwant %s\ngot %v", testObj.Metadata["MetaHeader"], val)
}
}
externalUrl := "" // We don't set any `externalURL` value during tests.
Copy link
Owner

Choose a reason for hiding this comment

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

super nit, but can you call this (and others) externalURL?

(I thought there was a vet rule for this in Go, but I guess I'm wrong)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing. Thanks for pointing it out; I wasn't aware.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 97841f3.

Comment on lines 146 to 147
SelfLink: externalUrl + "/storage/v1/b/" + obj.BucketName + "/o/" + obj.Name,
MediaLink: externalUrl + "/download/storage/v1/b/" + obj.BucketName + "/o/" + obj.Name + "?alt=media",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should probably URL-encode these. I'll take care of that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6c65141.

@manuteleco manuteleco requested a review from fsouza March 16, 2024 05:17
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!

@fsouza fsouza merged commit 2e2d14e into fsouza:main Mar 18, 2024
27 checks passed
@manuteleco manuteleco deleted the additional_object_response_attributes branch March 18, 2024 20:04
@mdedetrich
Copy link

@fsouza Is it possible to push a new docker image so we can use this change?, the last one is 2 months old and there are also other new changes which has gotten pushed.

@fsouza
Copy link
Owner

fsouza commented Mar 22, 2024

@fsouza Is it possible to push a new docker image so we can use this change?, the last one is 2 months old and there are also other new changes which has gotten pushed.

Yes, I'll publish a new release later today!

@mdedetrich
Copy link

@fsouza Is it possible to push a new docker image so we can use this change?, the last one is 2 months old and there are also other new changes which has gotten pushed.

Yes, I'll publish a new release later today!

Many thanks!

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.

Self link and media link missing
3 participants