Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consistent returns #13245

Merged
merged 17 commits into from
Aug 28, 2020
Merged

Consistent returns #13245

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
ccafe0f
fixes sync create_entity to return just the metadata
seankane-msft Aug 20, 2020
4b1f75a
fixes the create_table method to return just metadata
seankane-msft Aug 20, 2020
c21fd4c
fixes the create_table method to return just metadata
seankane-msft Aug 20, 2020
20cb21a
fixed update_entity too
seankane-msft Aug 20, 2020
e4de1d5
changed upsert_entity too, now to change the async methods
seankane-msft Aug 20, 2020
d7f1f00
fixed async create and update entity
seankane-msft Aug 20, 2020
7be28fc
fixed async create and update entity
seankane-msft Aug 20, 2020
94b7415
updated async methods to return metadata only, updated tests accordin…
seankane-msft Aug 20, 2020
ba82b25
tox fixes
seankane-msft Aug 20, 2020
1499b47
fixing merge conflicts
seankane-msft Aug 20, 2020
2a71664
merge conflicts round 2
seankane-msft Aug 21, 2020
56bfcdd
addressing izzys comments, fixing typehint formats, checking for a co…
seankane-msft Aug 24, 2020
1b0a191
fixed comments
seankane-msft Aug 24, 2020
1d52074
making the same change for async code
seankane-msft Aug 24, 2020
ade3d40
addressing annas comments, trimming metadata returns, removed if_not_…
seankane-msft Aug 25, 2020
c10926d
had an extra space in there causing pylint failure
seankane-msft Aug 25, 2020
49c0137
changed how _trim_service_metadata works
seankane-msft Aug 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions sdk/tables/azure-data-tables/azure/data/tables/_deserialize.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,3 +206,12 @@ def _return_headers_and_deserialized(response, deserialized, response_headers):

def _return_context_and_deserialized(response, deserialized, response_headers): # pylint: disable=unused-argument
return response.http_response.location_mode, deserialized, response_headers


def _trim_service_metadata(metadata):
# type: (dict[str,str] -> None)
return {
"date": metadata.pop("date", None),
"etag": metadata.pop("etag", None),
"version": metadata.pop("version", None)
}
97 changes: 55 additions & 42 deletions sdk/tables/azure-data-tables/azure/data/tables/_table_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from azure.core.exceptions import HttpResponseError, ResourceNotFoundError
from azure.core.tracing.decorator import distributed_trace

from ._deserialize import _convert_to_entity
from ._deserialize import _convert_to_entity, _trim_service_metadata
from ._entity import TableEntity
from ._generated import AzureTable
from ._generated.models import AccessPolicy, SignedIdentifier, TableProperties, QueryOptions
Expand All @@ -28,7 +28,7 @@
from ._deserialize import _return_headers_and_deserialized
from ._error import _process_table_error
from ._version import VERSION
from ._models import TableEntityPropertiesPaged, UpdateMode, TableItem
from ._models import TableEntityPropertiesPaged, UpdateMode


class TableClient(TableClientBase):
Expand Down Expand Up @@ -126,7 +126,7 @@ def get_table_access_policy(
self,
**kwargs # type: Any
):
# type: (...) -> dict[str,AccessPolicy]
# type: (...) -> Dict[str,AccessPolicy]
"""Retrieves details about any stored access policies specified on the table that may be
used with Shared Access Signatures.

Expand All @@ -148,7 +148,7 @@ def get_table_access_policy(
@distributed_trace
def set_table_access_policy(
self,
signed_identifiers, # type: dict[str,AccessPolicy]
signed_identifiers, # type: Dict[str,AccessPolicy]
**kwargs):
# type: (...) -> None
"""Sets stored access policies for the table that may be used with Shared Access Signatures.
Expand Down Expand Up @@ -180,17 +180,19 @@ def create_table(
self,
**kwargs # type: Any
):
# type: (...) -> TableItem
# type: (...) -> Dict[str,str]
"""Creates a new table under the current account.

:return: TableItem created
:rtype: TableItem
:return: Dictionary of operation metadata returned from service
:rtype: dict[str,str]
:raises: ~azure.core.exceptions.HttpResponseError
"""
table_properties = TableProperties(table_name=self.table_name, **kwargs)
try:
table = self._client.table.create(table_properties)
return TableItem(table=table)
metadata, _ = self._client.table.create(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's in this metadata? Do we need to clean out unnecessary information?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The returned metadata has these keys: client_request_id, request_id, version, date, and etag always. On create_entity the service also returns preference_applied and content_type. client_request_id, request_id, and preference_applied are all None in the tests

table_properties,
cls=kwargs.pop('cls', _return_headers_and_deserialized))
return _trim_service_metadata(metadata)
except HttpResponseError as error:
_process_table_error(error)

Expand Down Expand Up @@ -231,59 +233,57 @@ def delete_entity(
:raises: ~azure.core.exceptions.HttpResponseError
"""

if_match, if_not_match = _get_match_headers(kwargs=dict(kwargs, etag=kwargs.pop('etag', None),
if_match, _ = _get_match_headers(kwargs=dict(kwargs, etag=kwargs.pop('etag', None),
match_condition=kwargs.pop('match_condition', None)),
etag_param='etag', match_param='match_condition')
try:
self._client.table.delete_entity(
table=self.table_name,
partition_key=partition_key,
row_key=row_key,
if_match=if_match or if_not_match or '*',
if_match=if_match or '*',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a question: why are you no longer checking if_not_match

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if_not_match is not supported by the service right now.

**kwargs)
except HttpResponseError as error:
_process_table_error(error)

@distributed_trace
def create_entity(
self,
entity, # type: Union[TableEntity, dict[str,str]]
entity, # type: Union[TableEntity, Dict[str,str]]
**kwargs # type: Any
):
# type: (...) -> TableEntity
# type: (...) -> Dict[str,str]
"""Insert entity in a table.

:param entity: The properties for the table entity.
:type entity: Union[TableEntity, dict[str,str]]
:return: TableEntity mapping str to azure.data.tables.EntityProperty
:rtype: ~azure.data.tables.TableEntity
:return: Dictionary mapping operation metadata returned from the service
:rtype: dict[str,str]
:raises: ~azure.core.exceptions.HttpResponseError
"""

if "PartitionKey" in entity and "RowKey" in entity:
entity = _add_entity_properties(entity)
# TODO: Remove - and run test to see what happens with the service
else:
raise ValueError('PartitionKey and RowKey were not provided in entity')
try:
inserted_entity = self._client.table.insert_entity(
metadata, _ = self._client.table.insert_entity(
table=self.table_name,
table_entity_properties=entity,
**kwargs
)
properties = _convert_to_entity(inserted_entity)
return properties
cls=kwargs.pop('cls', _return_headers_and_deserialized),
**kwargs)
return _trim_service_metadata(metadata)
except ResourceNotFoundError as error:
_process_table_error(error)

@distributed_trace
def update_entity( # pylint:disable=R1710
self,
entity, # type: Union[TableEntity, dict[str,str]]
entity, # type: Union[TableEntity, Dict[str,str]]
mode=UpdateMode.MERGE, # type: UpdateMode
**kwargs # type: Any
):
# type: (...) -> None
# type: (...) -> Dict[str,str]
"""Update entity in a table.

:param entity: The properties for the table entity.
Expand All @@ -294,33 +294,41 @@ def update_entity( # pylint:disable=R1710
:keyword str row_key: The row key of the entity.
:keyword str etag: Etag of the entity
:keyword ~azure.core.MatchConditions match_condition: MatchCondition
:return: None
:rtype: None
:return: Dictionary mapping operation metadata returned from the service
:rtype: dict[str,str]
:raises: ~azure.core.exceptions.HttpResponseError
"""

if_match, if_not_match = _get_match_headers(kwargs=dict(kwargs, etag=kwargs.pop('etag', None),
if_match, _ = _get_match_headers(kwargs=dict(kwargs, etag=kwargs.pop('etag', None),
match_condition=kwargs.pop('match_condition', None)),
etag_param='etag', match_param='match_condition')

partition_key = entity['PartitionKey']
row_key = entity['RowKey']
entity = _add_entity_properties(entity)
try:
metadata = None
if mode is UpdateMode.REPLACE:
self._client.table.update_entity(
metadata, _ = self._client.table.update_entity(
table=self.table_name,
partition_key=partition_key,
row_key=row_key,
table_entity_properties=entity,
if_match=if_match or if_not_match or "*",
if_match=if_match or "*",
cls=kwargs.pop('cls', _return_headers_and_deserialized),
**kwargs)
elif mode is UpdateMode.MERGE:
self._client.table.merge_entity(table=self.table_name, partition_key=partition_key,
row_key=row_key, if_match=if_match or if_not_match or "*",
table_entity_properties=entity, **kwargs)
metadata, _ = self._client.table.merge_entity(
table=self.table_name,
partition_key=partition_key,
row_key=row_key,
if_match=if_match or "*",
table_entity_properties=entity,
cls=kwargs.pop('cls', _return_headers_and_deserialized),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@annatisch had mentioned before that she prefers something more like
cls=kwargs.pop('cls', None) or _return_headers_and_deserialized (can't remember this comment correctly, I think it might have had something to do with if the user passes in None for cls

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way it currently is if the user passes in None that will be passed to the service.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wont be passed to the service - as this parameter deals with client-side behaviour.
But yeah... this one is tricky, because if a user does provide a cls parameter, and it doesn't return a tuple of two values - the function will fail.....

**kwargs)
else:
raise ValueError('Mode type is not supported')
return _trim_service_metadata(metadata)
except HttpResponseError as error:
_process_table_error(error)

Expand Down Expand Up @@ -395,15 +403,15 @@ def get_entity(
row_key, # type: str
**kwargs # type: Any
):
# type: (...) -> TableEntity
# type: (...) -> Dict[str,str]
"""Queries entities in a table.

:param partition_key: The partition key of the entity.
:type partition_key: str
:param row_key: The row key of the entity.
:type row_key: str
:return: Entity mapping str to azure.data.tables.EntityProperty
:rtype: ~azure.data.tables.TableEntity
:return: Dictionary mapping operation metadata returned from the service
:rtype: dict[str,str]
:raises: ~azure.core.exceptions.HttpResponseError
"""
try:
Expand All @@ -420,19 +428,19 @@ def get_entity(
@distributed_trace
def upsert_entity( # pylint:disable=R1710
self,
entity, # type: Union[TableEntity, dict[str,str]]
entity, # type: Union[TableEntity, Dict[str,str]]
mode=UpdateMode.MERGE, # type: UpdateMode
**kwargs # type: Any
):
# type: (...) -> None
# type: (...) -> Dict[str,str]
"""Update/Merge or Insert entity into table.

:param entity: The properties for the table entity.
:type entity: Union[TableEntity, dict[str,str]]
:param mode: Merge or Replace and Insert on fail
:type mode: ~azure.data.tables.UpdateMode
:return: None
:rtype: None
:return: Dictionary mapping operation metadata returned from the service
:rtype: dict[str,str]
:raises: ~azure.core.exceptions.HttpResponseError
"""

Expand All @@ -441,25 +449,30 @@ def upsert_entity( # pylint:disable=R1710
entity = _add_entity_properties(entity)

try:
metadata = None
if mode is UpdateMode.MERGE:
self._client.table.merge_entity(
metadata, _ = self._client.table.merge_entity(
table=self.table_name,
partition_key=partition_key,
row_key=row_key,
table_entity_properties=entity,
cls=kwargs.pop('cls', _return_headers_and_deserialized),
**kwargs
)
elif mode is UpdateMode.REPLACE:
self._client.table.update_entity(
metadata, _ = self._client.table.update_entity(
table=self.table_name,
partition_key=partition_key,
row_key=row_key,
table_entity_properties=entity,
cls=kwargs.pop('cls', _return_headers_and_deserialized),
**kwargs)
else:
raise ValueError('Mode type is not supported')
raise ValueError("""Update mode {} is not supported.
For a list of supported modes see the UpdateMode enum""".format(mode))
return _trim_service_metadata(metadata)
except ResourceNotFoundError:
self.create_entity(
return self.create_entity(
partition_key=partition_key,
row_key=row_key,
table_entity_properties=entity,
Expand Down
Loading