aws-beam / aws-codegen

Code generator for AWS clients in Erlang and Elixir.
Other
53 stars 49 forks source link

[#50] Use maps in the generated Erlang code to handle optional parameters #64

Closed robertoaloi closed 3 years ago

robertoaloi commented 3 years ago

Currently, the generated Erlang code requires the user to pass all parameters (both the required and optional ones) to the respective functions, eventually using the undefined atom. As you can image, this makes the library useless for Erlang users when it comes to function which accept a significant number of arguments.

What I propose here is to use maps to pass the optional parameters, instead. Before investing too much time on it, I wanted to hear your opinion.

This PR allows to go from the current:

get_object(Client, Bucket, Key, PartNumber, ResponseCacheControl, ResponseContentDisposition, ResponseContentEncoding, ResponseContentLanguage, ResponseContentType, ResponseExpires, VersionId, ExpectedBucketOwner, IfMatch, IfModifiedSince, IfNoneMatch, IfUnmodifiedSince, Range, RequestPayer, SSECustomerAlgorithm, SSECustomerKey, SSECustomerKeyMD5)
  when is_map(Client) ->
    get_object(Client, Bucket, Key, PartNumber, ResponseCacheControl, ResponseContentDisposition, ResponseContentEncoding, ResponseContentLanguage, ResponseContentType, ResponseExpires, VersionId, ExpectedBucketOwner, IfMatch, IfModifiedSince, IfNoneMatch, IfUnmodifiedSince, Range, RequestPayer, SSECustomerAlgorithm, SSECustomerKey, SSECustomerKeyMD5, []).
get_object(Client, Bucket, Key, PartNumber, ResponseCacheControl, ResponseContentDisposition, ResponseContentEncoding, ResponseContentLanguage, ResponseContentType, ResponseExpires, VersionId, ExpectedBucketOwner, IfMatch, IfModifiedSince, IfNoneMatch, IfUnmodifiedSince, Range, RequestPayer, SSECustomerAlgorithm, SSECustomerKey, SSECustomerKeyMD5, Options)
  when is_map(Client), is_list(Options) ->
    Path = ["/", aws_util:encode_uri(Bucket), "/", aws_util:encode_multi_segment_uri(Key), ""],
    SuccessStatusCode = undefined,

    Headers0 =
      [
        {<<"x-amz-expected-bucket-owner">>, ExpectedBucketOwner},
        [...]
      ],
    Headers = [H || {_, V} = H <- Headers0, V =/= undefined],

    Query0_ =
      [
        {<<"partNumber">>, PartNumber},
        [...]
      ],
    Query_ = [H || {_, V} = H <- Query0_, V =/= undefined],

    [...]

To a slightly more user friendly:

 get_object(Client, Bucket, Key)
  when is_map(Client) ->
    get_object(Client, Bucket, Key, #{}, #{}).

get_object(Client, Bucket, Key, QueryMap, HeadersMap)
  when is_map(Client), is_map(QueryMap), is_map(HeadersMap) ->
    get_object(Client, Bucket, Key, QueryMap, HeadersMap, []).

get_object(Client, Bucket, Key, QueryMap, HeadersMap, Options)
  when is_map(Client), is_map(QueryMap), is_map(HeadersMap), is_list(Options) ->
    Path = ["/", aws_util:encode_uri(Bucket), "/", aws_util:encode_multi_segment_uri(Key), ""],
    SuccessStatusCode = undefined,

    Headers0 =
      [
        {<<"x-amz-expected-bucket-owner">>, maps:get(<<"x-amz-expected-bucket-owner">>, HeadersMap, undefined)},
        [...]
      ],
    Headers = [H || {_, V} = H <- Headers0, V =/= undefined],

    Query0_ =
      [
        {<<"partNumber">>, maps:get(<<"partNumber">>, QueryMap, undefined)},
        [...]
      ],
    Query_ = [H || {_, V} = H <- Query0_, V =/= undefined],

    [...]

A few considerations:

robertoaloi commented 3 years ago

One of the jobs timed out. Re-running for now to see if it's a deterministic error. Anyone who experienced that before?

jfacorro commented 3 years ago

I think this is one of the things the will make the library a lot more usable.

Regarding testing, sadly the way to test this currently means:

  1. Running the generation of the files.
  2. Replacing the modules in a local copy of the aws-erlang repository.
  3. Run the aws-erlang tests and checks to make sure everything compiles and there are no warnings.
  4. Start a rebar3 shell in aws-erlang, create a client and use it to perform some operation in the modified generated modules.

It's manual and it sucks, but it also provides a level of confidence that the code looks and behaves as expected.

robertoaloi commented 3 years ago

I successfully run the Kinesis test from the README, an S3 put and an SSM get and all seem to be fine. I found an encoding issue with S3 GET, but I believe that is not related to this PR, so I will proceed and merge this one.

We really need to get some automation and tests around this.