amazonka-core: 304 is the only successful redirect response

[endgame]

From #755:

If you use the wrong region and do a simple S3 GetObject, then the
response is a 301 redirect. The problem is that this library treats
this as a successful response, and returns the AWS XML redirect page
as the response body!

However, S3 returns 304 if a request matches the given ETag. Therefore, consider only 2xx and 304 responses as successful.

Closes #755

[endgame]

@brendanhay @bitc @mgjpy3 @pbrisbin @cs @romanb @AriFordsham you may have opinions or further context on this, since you've raised bugs or discussed S3 response codes over the years.

[bitc]

In my opinion, this is definitely an improvement, but I have two notes:

1. The original bug Bug: S3 GetObject considers 3xx HTTP status codes success #755 was about S3 service only. This PR appears to affect all AWS services. Are we sure that this PR won't adversely affect other services? (I think that it is OK, but worth verifying).
2. I am on the fence about 304 status code (specifically for S3 service, I am not familiar enough with other AWS services). I understand the argument of considering it a "successful" response, because the client must specifically opt in via an ETag and thus expect this response. But it still feels fuzzy to me whether 304 really constitutes a "successful" response. I'm not sure that it deserves special treatment over for example 301 response. Both 301 and 304 tell the client "further action is needed in order to retrieve the data" -- in the case of 301 the client should issue a new request to a new URL in order to retrieve the data, and in the case of 304 the client should access its local cache in order to retrieve the data.

In any case, overall this is a 👍 from me, but I am interested to hear what others have to say about this PR.

Thank you

[endgame]

@bitc

Re: Original bug: previous "fixes" have always been to statusSuccess and thus affected every AWS service. I think this was done to allow 304s to work with S3 (#506).

It looks like newEnv creates a http-client Manager with pretty default settings, which includes "follow redirects". I can't see anywhere that this changes, and yet... we don't seem to be able to follow redirects returned from S3?

@pbrisbin I am replying to your comment here to keep discussion in one place.

I see two ways to decide whether a request is successful:

Just to confirm some assumptions of mine: [snip]

I think that these are correct.

If this is all true, I believe my suggestion above is still best. It's a version of (1), but maybe diverges on details?

* Define `_svcCheck = statusIsSuccessful || statusIsRedirection` for `S3`

This breaks the S3 client, because of #755 - S3 GetObject returns 301 with an XML error as a body, and then amazonka merrily considers that the actual response.

Also, the generator currently hardcodes the generation of the _serviceCheck field - not a blocker for the idea, but something to consider.

---

I think it is safe to generally consider 304s as successful, since you need to set some kind of header like ETag before 304 responses are possible.

The classification of status codes is already provided by http-types, so we don't need to add those.

Currently I'm weakly against adding generator-level knobs to generate different predicates because I think that this PR is likely to be enough. But I also want to let this discussion stew for a couple of days in case I'm wrong.

[pbrisbin]

This breaks the S3 client, because of #755 - S3 GetObject returns 301 with an XML error as a body, and then amazonka merrily considers that the actual response.

I must be missing something because you lost me here. I thought the behavior on current main was intentional, desired, and it was to indeed treat 3XX as success and "merrily" proceed. I was only trying to keep to that in my suggestion (with convenient ergonomics of opting out as an end-user). If you're agreeing that's "broken", then why not simply revert back to the 1.6 behavior? I could take or leave the special handling of 304 personally.

Also, the generator currently hardcodes the generation of the _serviceCheck field

I think this is a totally valid criticism of my approach, and a good reason to just do something uniform all the time, whatever it is.

[endgame]

What I think was desired by that change was to treat 304 as success for people who were trying to check cached content, and the 301 bug was an undesired consequence.

[pbrisbin]

I see! I hadn't seen that stated that clearly in any of the threads so far. Well, this PR seems perfect then.

[endgame]

Nobody else has said anything, so I'll merge this now. I will be online for another 2–3 days and then unreachable for about a week. If you have objections, please raise them promptly.