Protocols trait support #3376
Protocols trait support #3376
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #3376 +/- ##
==========================================
Coverage ? 93.05%
==========================================
Files ? 66
Lines ? 14491
Branches ? 0
==========================================
Hits ? 13485
Misses ? 1006
Partials ? 0 ☔ View full report in Codecov by Sentry. |
botocore/args.py
Outdated
| def _resolve_protocol(self, service_model): | ||
| # If a service does not have a `protocols` trait, fall back to the legacy | ||
| # `protocol` trait | ||
| if 'protocols' not in service_model.metadata: |
There was a problem hiding this comment.
This one feels a little odd to me. We may want to be validating the truthiness of protocols rather than the key existing. It should be equivalent or a supeset of protocols, but if it's [] and protocol exists, it seems better to be using the protocol instead of hard failing?
There was a problem hiding this comment.
I don't think we can access any of the new CachedProperty properties directly unless we are somehow guaranteed they will be there in all models. This guarantee is impossible because some users have custom models, so we will always need some way to fall back to the existing properties. The reason is that when we access any of these properties- service_model.protocols for instance, it calls _get_metadata_property under the hood, which will raise an error if the key doesn't exist:
def _get_metadata_property(self, name):
try:
return self.metadata[name]
except KeyError:
raise UndefinedModelAttributeError(
f'"{name}" not defined in the metadata of the model: {self}'
)
Also, while I'm not sure how we could get into the codepath where properties is an empty array given that we have a test below that prevents that, I think that the readability of this code could be improved by Nate's suggestion, so I'll go make that change and push up a revision shortly :)
I'm not sure that there's a way to safely call service_model.protocols directly though, so I'll have to do it through service_model.metadata.get('protocol')
| services_models_with_protocols.append(service_model) | ||
| else: | ||
| services_models_without_protocols.append(service_model) | ||
| return (services_models_with_protocols, services_models_without_protocols) |
There was a problem hiding this comment.
[Not Blocking]This seems pretty memory inefficient as opposed to using yield. I don't feel strongly though given this pattern is used in other tests and this isn't actually SDK code.
There was a problem hiding this comment.
I've updated it to something "better"? It doesn't load them all into memory, but it feels a bit awkward. I'd love some feedback on it when you get the chance.
Co-authored-by: jonathan343 <[email protected]>
Co-authored-by: Nate Prewitt <[email protected]>
Co-authored-by: jonathan343 <[email protected]>
SamRemis
force-pushed
the
protocols-trait-support
branch
from
f51ee8e to
653e512
Compare
* release-1.36.19: Bumping version to 1.36.19 Update to latest models Protocols trait support (#3376)
Add support for protocols trait Co-authored-by: jonathan343 <[email protected]> Co-authored-by: Nate Prewitt <[email protected]>
Add support for protocols trait Co-authored-by: jonathan343 <[email protected]> Co-authored-by: Nate Prewitt <[email protected]>
This PR adds support for the new
protocolstrait, which allows a service to list a number of protocols it supports; botocore will automatically choose the highest priority supported protocol and use that to make requests to the service.