feat: Middleware.PathParams: add OpenAPI params

[halostatue]

Tesla.Middleware.PathParams has been extended to support OpenAPI style parameters in addition to the existing Phoenix style parameters.

• Phoenix parameters begin with : and a letter (a-zA-Z) and continue to the next non-word character (not a-zA-Z0-9_). Examples include :id, :post_id, :idPost.

• OpenAPI parameters are contained in braces ({}), must begin with a letter (a-zA-Z) and may contain word characters and hyphens (-a-zA-Z0-9_). Examples inlucde {id}, {post_id}, {IdPost}.

Parameter value objects may be provided as maps with atom or string keys, keyword lists, or structs. During path resolution, to avoid potential atom exhaustion, the parameter value object is transformed to a string-keyed map.

Parameter values that are nil or that are not present in the value object are not replaced; all other values are converted to string (with to_string) and encoded with application/x-www-form-urlencoded formatting. If the value does not implement the String.Chars protocol, it is the caller's responsibility to convert it to a string prior to passing it to the Tesla client using this middleware.

Execution time is determined by the number of parameter patterns in the path.

Closes: #566

[yordis]

Related to #566 as well

This does not handle "nested" OpenAPI-style parameters well (that is, {post{id}} or {id{post}}), and I'm not sure what the behaviour here should be.

I couldn't find docs about that in the Open API spec. Do you mind linking where is this possible? I didn't know that.
That situation seems weird anyway, most people would flat that to post_id

This mostly follows the Phoenix implementation for path parameters, excepting that capital letters are permitted. This matches the previous version's implementation.
Try to avoid breaking changes, we can always document the ideal implementation and wait until we have the opportunity to do so.

I would personally want to have different parameter identifier matching rules for :id vs {id} formats, but that gets complicated.

I would like {name} to stick to whatever Open API spec says.

This does not handle "nested" OpenAPI-style parameters well (that is, {post{id}} or {id{post}}), and I'm not sure what the behaviour here should be. Strictly speaking, we should probably be doing this somewhat differently than the logic currently implemented or the previous regex-based logic:

It is OK we have a working version without "nesting", and yes, we should move away from regex if possible, I like your proposal for the macro 🤯

[halostatue]

Related to #566 as well

This does not handle "nested" OpenAPI-style parameters well (that is, {post{id}} or {id{post}}), and I'm not sure what the behaviour here should be.

I couldn't find docs about that in the Open API spec. Do you mind linking where is this possible? I didn't know that. That situation seems weird anyway, most people would flat that to post_id

I don't believe it is legal, but the implementation in this PR is naïve and will handle it oddly. "{post{id}}" with [post: 3, id: 2] will result in "{post2}" whereas "{id{post}}" will result in "{id3}". This gets particularly ugly if you do the latter with [post: 3, id: 2, id3: 5], which will result in "5". It's been a little while since I wrote the code and the PR message, but I believe I was trying to point out limitations.

This mostly follows the Phoenix implementation for path parameters, excepting that capital letters are permitted. This matches the previous version's implementation.
Try to avoid breaking changes, we can always document the ideal implementation and wait until we have the opportunity to do so.

I would personally want to have different parameter identifier matching rules for :id vs {id} formats, but that gets complicated.

I would like {name} to stick to whatever Open API spec says.

That is ugly. 'The value for these path parameters MUST NOT contain any unescaped "generic syntax" characters described by RFC3986: forward slashes (/), question marks (?), or hashes (#).'

In other words, I think that {0} would be permitted.

This does not handle "nested" OpenAPI-style parameters well (that is, {post{id}} or {id{post}}), and I'm not sure what the behaviour here should be. Strictly speaking, we should probably be doing this somewhat differently than the logic currently implemented or the previous regex-based logic:

It is OK we have a working version without "nesting", and yes, we should move away from regex if possible, I like your proposal for the macro 🤯

The macro approach has definite downsides, especially as the macro-style of Tesla client declaration is disfavoured (I am slowly trying to convert everything to runtime configured) and it would require Tesla's URL handling to be aware of such an iodata-ish approach.

[yordis]

Keep going, get to a place where you are happy with things, open for code review, and we open up the discussion to figure out what should be the end-state.

Thank you so much for the contribution thus far!

[halostatue]

I’ll do a quick once over this weekend; we are currently using a variant of this in production and it works very nicely, as long as you don't do silly things.

[halostatue]

Sorry for the delays, but this is now ready. There were a couple of issues we found in practice that I put back in place.

[yordis]

It looks great so far, I left couple questions about it.

[halostatue]

I have a better fix in a second commit and will squash before merging.

[yordis]

I do not hate it thou!

Maybe avoiding the regex and add compile-time URL contractors that could speed up the situation?

But overall, it seems good to me!

[yordis]

Anything last words? ?

[halostatue]

We shouldn't see anything much worse in performance because the current solution uses a similar Regex.replace. I’m going to squash it down and include a reference that this closes #566 (because it does!). I will reword the unified commit message for the next release notes.

[halostatue]

Squashed and main PR message ready to go.

I think that having a mechanism to tokenize URLs / URL paths and pass that to Tesla is a slightly larger discussion because it is more invasive and may have negative impacts on other plug-ins if not supported. Probably better for a 2.0 discussion, but it would make specific path matching easier and more reliable, IMO.

Although the code here is different than what we're using in production, it’s somewhat closer to the original implementation and has nothing but upsides, IMO.

[yordis]

🚀 💜