Fixes Bad Request to more clear error #8
Conversation
…nd TableServiceClient
| @@ -56,6 +57,12 @@ def __init__( | |||
| :returns: None | |||
| """ | |||
|
|
|||
| if re.match("^[a-zA-Z]{1}[a-zA-Z0-9]{2,62}$", table_name) is None: | |||
| raise HttpResponseError( | |||
There was a problem hiding this comment.
generally getting an HttpResponseError will signify to the user that their network call was inncorrect. In this case, no network call is made, it's more an issue with the value that the user passed for the table name. I think raising a ValueError will be better in this case
| invalid_table_name = "$&#*_(%&@*(_(" | ||
|
|
||
| # Assert | ||
| with self.assertRaises(HttpResponseError): |
There was a problem hiding this comment.
I think you should also add an assert that the thrown error has a message that contains "Table names must be alphanumeric, cannot begin with a number, and must be between 3-63 characters long."
| @@ -33,10 +33,10 @@ or [Azure CLI](https://docs.microsoft.com/azure/storage/common/storage-quickstar | |||
| ```bash | |||
| # Create a new resource group to hold the storage account - | |||
| # if using an existing resource group, skip this step | |||
| az group create --name my-resource-group --location westus2 | |||
| az group create --name MyResourceGroup --location westus2 | |||
There was a problem hiding this comment.
In the future, I think your PRs should be a little more tailored, i.e. the readme changes should be a separate PR from the error fix, since they're not the same issue. No need to fix this PR though
sdk/table/azure-table/README.md
Outdated
| @@ -158,16 +158,16 @@ Create a table in your storage account | |||
| ```python | |||
| from azure.table import TableServiceClient | |||
|
|
|||
| table = TableServiceClient.from_connection_string(conn_str="<connection_string>") | |||
| table.create_table(table_name="myTable") | |||
| client = TableServiceClient.from_connection_string(conn_str="<connection_string>") | |||
There was a problem hiding this comment.
nit: in scenarios with multiple clients, I like having table_service_client and table_client as the variable names for the 2 idfferent clients
sdk/table/azure-table/README.md
Outdated
| @@ -178,7 +178,7 @@ from azure.table import TableClient | |||
|
|
|||
| my_entity = {'PartitionKey':'part','RowKey':'row'} | |||
|
|
|||
| table = TableClient.from_connection_string(conn_str="<connection_string>", table_name="my_table") | |||
| table = TableClient.from_connection_string(conn_str="<connection_string>", table_name="myTable") | |||
| entity = table.create_entity(table_entity_properties=my_entity) | |||
There was a problem hiding this comment.
Libba recently renamed the parameter table_entity_properties to just entity
sdk/table/azure-table/README.md
Outdated
| @@ -189,7 +189,7 @@ from azure.table.aio import TableClient | |||
|
|
|||
| my_entity = {'PartitionKey':'part','RowKey':'row'} | |||
There was a problem hiding this comment.
we don't have to do this now, but might be interesting to have some other keys as part of the entity:
entity = {
'PartitionKey': 'color',
'RowKey': 'brand',
'text': 'Marker',
'color': 'Purple',
'price': '5'
}
sdk/table/azure-table/README.md
Outdated
| @@ -199,7 +199,7 @@ Querying entities in the table | |||
| ```python | |||
| from azure.table import TableClient | |||
|
|
|||
| table = TableClient.from_connection_string(conn_str="<connection_string>", table_name="my_table") | |||
| table = TableClient.from_connection_string(conn_str="<connection_string>", table_name="mytable") | |||
| entity = table.query_entities(results_per_page=3) | |||
There was a problem hiding this comment.
pretty sure query_entities takes a required positional parameter filter. @LibbaLawrence can confirm.
Or maybe we want this sample to demonstrate list_entities? In which case we should update the method name.
| @@ -196,6 +203,12 @@ def delete_table( | |||
| :return: None | |||
| :rtype: None | |||
| """ | |||
| if re.match("^[a-zA-Z]{1}[a-zA-Z0-9]{2,62}$", table_name) is None: | |||
There was a problem hiding this comment.
wonder if we can refactor this code into a function like is_valid_table_name(table_name) to reduce duplicate code across clients
|
@iscai-msft @kristapratico I've addressed all the comments mentioned above. I'll leave the major readme and samples updates for a different PR. |
| def test_create_table_invalid_name(self, resource_group, location, storage_account, storage_account_key): | ||
| # Arrange | ||
| ts = TableServiceClient(self.account_url(storage_account, "table"), storage_account_key) | ||
| # table_name = self._get_table_reference() |
There was a problem hiding this comment.
nit: I'd remove this commented out code
| ts = TableServiceClient(self.account_url(storage_account, "table"), storage_account_key) | ||
| # table_name = self._get_table_reference() | ||
| # btable_client = ts.get_table_client(table_name) | ||
| invalid_table_name = "$&#*_(%&@*(_(" |
There was a problem hiding this comment.
i wonder if it'd be good to switch the invalid table name to the one that you initially found this error with. i.e. 'my-table', since it's a more common mistake for users to make. Sorry should've mentioned this one earlier
There was a problem hiding this comment.
I think you're right, I'll change that. Users aren't as likely to make a table name $&#*_(%&@*(_( as they are my_table
Fixes Azure#12541