brendanhay / amazonka

A comprehensive Amazon Web Services SDK for Haskell.
https://amazonka.brendanhay.nz
Other
599 stars 227 forks source link

s3: make the timestamps use rfc822 #881

Closed divarvel closed 1 year ago

divarvel commented 1 year ago

depends on #882

todo: regen S3 after #882 is merged and a tree-wide regen is done

AWS does not explicitly specify that it is the required format, but RFC3339-formatted dates are ignored, all dates are formatted with RFC822 in responses, and examples also use RFC822 dates. Considering the age of S3, it makes sense.

Fixes #879

As for annotating S3 shapes, i've split the change in two commits. The first one is for fields where i'm reasonably sure it's okay (because i've tested it), the second one for fields where i'm less sure, even though it makes sense. All cache related headers tend to be RFC822-formatted since they are specified by HTTP RFCs, which require this format.

divarvel commented 1 year ago

Running ./scripts/generate --commit s3 outputs a bunch of stuff (most notably, it says that it wrote .hs files in a temp directory), but no commit is generated (and the working copy is not affected either).

Overriding type to something else (like string) in the annex file makes the codegen update the files as expected, so i'm guessing the issue is with timestampFormat not being used by the code generator (at least in shapes).

Indeed, setting the timestamp format to rfc822 in the S3 service metadata works (but affects everything). S3 seems to use RFC822 dates in headers and ISO8601 dates in JSON payloads, so setting RFC822 dates everywhere seems a bit extreme.

Reading the code in gen/, it looks like timestampFormat is only read from service metadata. I have not found yet where shapes are turned into haskell types, to see whether support for an optional timestampFormat could be easily added.

endgame commented 1 year ago

AFAICT you have written the annex correctly and that neither existing configs nor botocore set it in service metadata. That field should be moved from service metadata to the parser for individual types - try adding a Timestamp field to the Time constructor of Lit in Ann.hs, and fixing up the instance FromJSON (ShapeF ()).

divarvel commented 1 year ago

Well, that worked. I had to move Timestamp to Ann.hs and had the Time constructor take a Maybe Timestamp.

Now that i have something that works, i'll dig a bit more to see if i can annotate other shapes.

divarvel commented 1 year ago

Interestingly, the description file has a couple explicit timestampFormat values set (a few ISO8601, and a single RFC822 that is correctly picked up by codegen).

Maybe the first commit will warrant a tree-wide regen, since other services might be affected.

endgame commented 1 year ago

Yes, I think a tree-wide regeneration is in order. Can you please:

  1. Remove the s3 regeneration from this branch
  2. Add an entry to lib/amazonka/CHANGELOG.md (which is the master changelog for the project)

I will merge #878 and regenerate everything once that's done. I'd hoped to get a fix in for #880 before I next ran the generator, but that seems a lot harder than I first thought, so we'll gen once the S3 fixes are in.

divarvel commented 1 year ago

Alrighty, will do in the morning (CET)

endgame commented 1 year ago

I think we should remove the timestampFormat field from the Metadata type, and hardcode the fallback in Syntax.hs - nothing in botocore declares anything there, and neither do any of our annexes.

endgame commented 1 year ago

I think this will entail moving the check from substitute (Subst.hs) into Syntax.directed, which should be fine if you specialise the HasMetadata a Identity into Service Identity a b c. Then you can remove the timestampFormat stuff from the metadata record - I've never seen that field populated in botocore or in any of our annexes.

divarvel commented 1 year ago

I have added a changelog entry for this one, i'll add a changelog entry for #882 as well.