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: Support REST responses with their own ETag field. #680

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

amanda-tarafa
Copy link
Contributor

Unblocks releasing Google.Apis.Contentwarehouse.v1

@jskeet as usual, a commit at a time so that generation changes are clear. I'll probably rebase into a single commit before merging though, the first commit won't even build.

@amanda-tarafa amanda-tarafa requested a review from a team as a code owner September 12, 2023 04:20
@amanda-tarafa amanda-tarafa marked this pull request as draft September 12, 2023 05:15
@amanda-tarafa
Copy link
Contributor Author

Converting to draft, not yet ready for review.

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

So if both ETag and ETagValue are sent, presumably we'll end up with

{
  "etag": "value1",
  "ETag": "value2"
}

Is that what the API is expecting?

@amanda-tarafa
Copy link
Contributor Author

So if both ETag and ETagValue are sent, presumably we'll end up with

{
  "etag": "value1",
  "ETag": "value2"
}

Is that what the API is expecting?

In principle yes, although we've only seen this on a Response with only an "ETag" but not an "etag". But, since the Response is a top level model without an "etag" we generate an ETag property which is the one that clashes with the one that is normally generated from the "ETag".

Happy to chat about this more. I don't think it's too risky, but definetely some.
At the moment this change has no effect except on Content Warehouse V1, I just ran generation for all and there were no changes.

@amanda-tarafa amanda-tarafa marked this pull request as ready for review September 12, 2023 06:31
@amanda-tarafa
Copy link
Contributor Author

Marking as ready for review as everything generated fine locally.

But I'm more than happy to reconsider.

@jskeet
Copy link
Collaborator

jskeet commented Sep 12, 2023

I'm wondering whether we shouldn't just not autogenerate the existing one - which could be done by changing the situation under which we generate it. Let's chat about it when we're next meeting.

@jskeet
Copy link
Collaborator

jskeet commented Sep 13, 2023

I'm still nervous about how things will be parsed if we get both "etag" and "ETag" in the response - it would probably be good for us to test that. (Json.NET is case-insensitive by default; I can't remember whether we change that.)

Beyond that, I think we just need to bike-shed on the name. I think "ETagData" might be clearer than "ETagValue", but both are a bit tricky. Alternatively, to avoid any possibility of other collisions, we could just use "ETag_" or "ETag__" in the same way that we escape other things.

@amanda-tarafa
Copy link
Contributor Author

Yep, I'll add tests for having two etags on the JSON.

Re naming, I actually like the "__" approach better, it doesn't put on the spot about picking a name for the property and the field documentation can give a way better explanation of what it represents than we can ever capture with a name.

I'll make those changes and push shortly.

Unblocks releasing of Google.Apis.Contentwarehouse.v1.
@amanda-tarafa
Copy link
Contributor Author

I've changed to use "ETag__" and created googleapis/google-api-dotnet-client#2531 for testing the default JSON serialization behaviour. PTAL.

Copy link
Collaborator

@jskeet jskeet left a comment

Choose a reason for hiding this comment

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

Great, thanks - and thanks for the test in the other PR.

@amanda-tarafa amanda-tarafa merged commit 590e4de into googleapis:main Sep 14, 2023
4 checks passed
@amanda-tarafa amanda-tarafa deleted the rest-duplicate-etag branch September 14, 2023 21:01
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.

2 participants