Added client call decorator feature#1200
Conversation
Fixes smithy-lang#1197 Co-Authored-By: Michael Dowling <michael@mtdowling.com>
…s in call decorators
| * | ||
| * @return the context. | ||
| */ | ||
| public Context context() { |
There was a problem hiding this comment.
One potential "issue" of this change I see is that call decorators now would be able to modify the context (so it is not a readonly), but maybe it is not an issue as the context is being exposed to hooks already where they could do the same before).
There was a problem hiding this comment.
Yeah, it currently kind of blurs the line between override and given context. Maybe we just add one more argument to the two interfaces and pass Context? It’s a lot of arguments, but seems necessary (and I want to avoid a kind of wrapper allocation for this since it’ll be a hot path).
There was a problem hiding this comment.
I was thinking about it and it felt like it might be a bit ambiguous as in hooks we have context that is "merged" (or whichever takes precedence) with client's default context, but here we're mostly interested in the override context? Maybe its just me.
Another thought, do you think we should keep the RequestOverrideConfig in the arguments of ClientCallDecorator and ClientCallInvoker despite the fact that you can't do anything with it outside of smithy-java other than pass it down? But on the other side removing it from there would require to create unnecessary wrapping around doCall method...
There was a problem hiding this comment.
Maybe its just me.
I guess thats fine since the implementation can always take client's current context and do its own logic for "overrides" if needed, at least for now.
There was a problem hiding this comment.
@mtdowling pushed the changes in 1d6e17f and 669f55d
There was a problem hiding this comment.
Thanks. Gonna take a deeper look at this. Wondering if we can just not send RequestOverride somehow by resolving stuff before the decorator. Alternatively, expose ClientCall or a stripped down version of as an interface that exposes less than the internal ClientCall. Not sure.
| * | ||
| * @return the context. | ||
| */ | ||
| public Context context() { |
There was a problem hiding this comment.
Yeah, it currently kind of blurs the line between override and given context. Maybe we just add one more argument to the two interfaces and pass Context? It’s a lot of arguments, but seems necessary (and I want to avoid a kind of wrapper allocation for this since it’ll be a hot path).
…ow access in call decorators" This reverts commit 72dc964.
2 participants
Issue #, if available: fixes #1197
Description of changes:
Added support for client call decorator override described in #1197 (comment).
Also markedRequestOverrideConfig.contextso that it would become available in call decorators (happy to remove/change it if you think this is not what should be available).By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.