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.09%
==========================================
Files ? 66
Lines ? 14494
Branches ? 0
==========================================
Hits ? 13493
Misses ? 1001
Partials ? 0 ☔ View full report in Codecov by Sentry. |
| # If a service does not have a `protocols trait`, fall back to the legacy | ||
| # `protocol` trait | ||
| if 'protocols' not in service_model.metadata: | ||
| return service_model.metadata['protocol'] |
There was a problem hiding this comment.
super nit - We might want to switch over to using protocol property on the service model for consistency with the following lines that do a similar access on protocols:
service_model.protocolthis matches what we do in subsequent lines:
service_model.protocolsThere was a problem hiding this comment.
I considered this and chose not to since it was more consistent with what we were previously doing. Technically that could break a test somewhere if someone was mocking service_model.metadata['protocol'] but not service_model.protocol.
But stylistically, I agree with you. Knowing the reasoning behind this, which do you prefer? Do you think that the tradeoff is acceptable here?
There was a problem hiding this comment.
I kind of agree with using the properties. If users are stubbing out the metadata specifically and not the actual functionality on the client, that's going to cause a number of issues. I think we should be alright to simplify this a little.
There was a problem hiding this comment.
I'm kind of confused now lol.
If the metadata was being mocked, wouldn't the property return what they expect since it calls metadata under the hood?
@CachedProperty
def protocol(self):
return self._get_metadata_property('protocol')
...
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}'
)There was a problem hiding this comment.
That's what I pointed out to Sam. He said there's a case this wouldn't work like that (I'm not sure how), but even if that's the case, I don't think it's reasonable to expect that as a reasonable mock.
| @@ -823,3 +823,12 @@ class InvalidChecksumConfigError(BotoCoreError): | |||
| 'Unsupported configuration value for {config_key}. ' | |||
| 'Expected one of {valid_options} but got {config_value}.' | |||
| ) | |||
|
|
|||
|
|
|||
| class NoSupportedProtocolError(BotoCoreError): | |||
There was a problem hiding this comment.
What do you think of a name like UnsupportedServiceProtocolError instead?
There was a problem hiding this comment.
Naming errors like this is hard. It's really "no supported protocol" because there may be multiple as opposed to only one protocol not being supported. Maybe NoServiceProtocolsSupportedError? If you like UnsupportedServiceProtocolError better, I'm open to discussion.
There was a problem hiding this comment.
UnsupportedServiceProtocolsError is another alternative, I just want to make sure that protocols is plural
Co-authored-by: jonathan343 <43360731+jonathan343@users.noreply.github.com>
| { | ||
| "type": "enhancement", | ||
| "category": "Protocols", | ||
| "description": "This version of botocore adds support for using a priority ordered list of protocols to determine which protocol to use for a service" |
There was a problem hiding this comment.
nit; I would maybe simplify the message. This is probably what's relevant to the end user.
| "description": "This version of botocore adds support for using a priority ordered list of protocols to determine which protocol to use for a service" | |
| "description": "Added support for multiple protocols within a service based on performance priority." |
| # If a service does not have a `protocols trait`, fall back to the legacy | ||
| # `protocol` trait | ||
| if 'protocols' not in service_model.metadata: | ||
| return service_model.metadata['protocol'] |
There was a problem hiding this comment.
I kind of agree with using the properties. If users are stubbing out the metadata specifically and not the actual functionality on the client, that's going to cause a number of issues. I think we should be alright to simplify this a little.
| 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?
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.