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

[Cosmos] Add option for disabling auto id generation in create_item #15715

Merged
merged 12 commits into from
Jan 15, 2021
Merged

[Cosmos] Add option for disabling auto id generation in create_item #15715

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
cf925cb
Add option for disabling auto id generation in create_item
zfoster Dec 8, 2020
937171f
Adds ID generation to kwargs, removes bad test
zfoster Dec 10, 2020
56f9b58
Default disableAutoID gen to true, add back test
zfoster Dec 14, 2020
4028dd0
use correct kwarg
zfoster Dec 14, 2020
fa3a9fe
Fix test typos
zfoster Dec 14, 2020
262e672
Change flag to enable instead of disable
zfoster Jan 8, 2021
c6ca91c
Adds test for item create with no ID
zfoster Jan 8, 2021
c12d19d
disable header flag
zfoster Jan 8, 2021
0c99eaa
Merge branch 'zf/auto-id-gen' of https://github.com/zfoster/azure-sdk…
zfoster Jan 11, 2021
1070de4
Fixes auto ID generation in create_item
zfoster Jan 11, 2021
d8c763f
Merge branch 'master' into zf/auto-id-gen
zfoster Jan 11, 2021
9e8a247
Remove print and unnecessary passthrough_args
zfoster Jan 12, 2021
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
10 changes: 6 additions & 4 deletions sdk/cosmos/azure-cosmos/azure/cosmos/_cosmos_client_connection.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1072,20 +1072,22 @@ def CreateItem(self, database_or_container_link, document, options=None, **kwarg
# not each time the function is called (like it is in say, Ruby). This means
# that if you use a mutable default argument and mutate it, you will and have
# mutated that object for all future calls to the function as well. So, using
# a non-mutable deafult in this case(None) and assigning an empty dict(mutable)
# a non-mutable default in this case(None) and assigning an empty dict(mutable)
# inside the method For more details on this gotcha, please refer
# http://docs.python-guide.org/en/latest/writing/gotchas/
if options is None:
options = {}

# We check the link to be document collection link since it can be database
# link in case of client side partitioning
if base.IsItemContainerLink(database_or_container_link):
options = self._AddPartitionKey(database_or_container_link, document, options)

collection_id, document, path = self._GetContainerIdWithPathForItem(
database_or_container_link, document, options
)

if base.IsItemContainerLink(database_or_container_link):
print('checking link', document)
Copy link
Member

Choose a reason for hiding this comment

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

A print statement ;)
Additionally - why was this block moved down? Does the ordering change here affect the options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, I thought that print statement was already there when I reordered them. Let me remove! The order changed because the ID is generated and added to the body in the statement above this one. This statement then adds the partitionKey to the options. In the case that partitionKey is /id, we won't have the partitionKey in the options unless the statements are in this order

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

options = self._AddPartitionKey(database_or_container_link, document, options)

return self.Create(document, path, "docs", collection_id, None, options, **kwargs)

def UpsertItem(self, database_or_container_link, document, options=None, **kwargs):
Expand Down
7 changes: 5 additions & 2 deletions sdk/cosmos/azure-cosmos/azure/cosmos/container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,7 @@ def create_item(
:param pre_trigger_include: trigger id to be used as pre operation trigger.
:param post_trigger_include: trigger id to be used as post operation trigger.
:param indexing_directive: Indicate whether the document should be omitted from indexing.
:keyword bool enable_automatic_id_generation: Enable automatic id generation if no id present.
:keyword str session_token: Token for use with Session consistency.
:keyword dict[str,str] initial_headers: Initial headers to be sent as part of the request.
:keyword str etag: An ETag value, or the wildcard character (*). Used to check if the resource
Expand All @@ -498,7 +499,7 @@ def create_item(
request_options = build_options(kwargs)
response_hook = kwargs.pop('response_hook', None)

request_options["disableAutomaticIdGeneration"] = True
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned above - we probably need to keep this here to ensure consistent behaviour for existing customers, allowing the parameter overwrite:

request_options["disableAutomaticIdGeneration"] = kwargs.pop('disable_auto_id_gen', True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that makes sense - I was thinking this wouldn't affect users since it requires them to use an ID already, but I can see people relying on getting an error when they do not pass an ID

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @zfoster - just to clarify, when you say "requires them to use an ID already", whereabouts is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

request_options["disableAutomaticIdGeneration"] = True

having this hardcoded means you have to supply an ID in the args when creating items

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if i'm reading it right

Copy link
Member

Choose a reason for hiding this comment

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

Interesting - in that case what is the keyword argument people would need to have been supplying? And is that what our tests are doing? (sorry I should have dug deeper into this earlier)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we didn't have a kwarg for it afaict. honestly, I'm not sure why this is the way it is, it's unique to this sdk. Our tests do supply IDs and from this we can assume our users do too, however I can flesh out why I think your comment about ensuring consistent behavior is still important:

We may have users who generate IDs or create them programmatically, and rely on the fact that we don't ever automatically generate them. The way this is set up now, we keep this consistent and never generate IDs unless they explicitly pass the kwarg in (same behavior as before)

request_options["disableAutomaticIdGeneration"] = not kwargs.pop('enable_automatic_id_generation', False)
if populate_query_metrics:
request_options["populateQueryMetrics"] = populate_query_metrics
if pre_trigger_include is not None:
Expand All @@ -508,8 +509,10 @@ def create_item(
if indexing_directive is not None:
request_options["indexingDirective"] = indexing_directive

passthrough_args = {k: v for k, v in kwargs.items() if k != "enable_automatic_id_generation"}
Copy link
Member

@annatisch annatisch Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This line shouldn't be necessary as enable_automatic_id_generation has already been popped from the kwargs above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I think I saw this error before I understood what the difference between the whl and sdist tests were. I never rebuilt so I kept getting the error :) thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


result = self.client_connection.CreateItem(
database_or_container_link=self.container_link, document=body, options=request_options, **kwargs
database_or_container_link=self.container_link, document=body, options=request_options, **passthrough_args
)
if response_hook:
response_hook(self.client_connection.last_response_headers, result)
Expand Down
11 changes: 10 additions & 1 deletion sdk/cosmos/azure-cosmos/test/test_crud.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,15 @@ def test_document_crud(self):
# create a document
before_create_documents_count = len(documents)

# create a document with auto ID generation
document_definition = {'name': 'sample document',
'spam': 'eggs',
'key': 'value'}

created_document = created_collection.create_item(body=document_definition, enable_automatic_id_generation=True)
self.assertEqual(created_document.get('name'),
document_definition['name'])

document_definition = {'name': 'sample document',
'spam': 'eggs',
'key': 'value',
Expand All @@ -790,7 +799,7 @@ def test_document_crud(self):
documents = list(created_collection.read_all_items())
self.assertEqual(
len(documents),
before_create_documents_count + 1,
before_create_documents_count + 2,
'create should increase the number of documents')
# query documents
documents = list(created_collection.query_items(
Expand Down
10 changes: 5 additions & 5 deletions sdk/cosmos/azure-cosmos/test/test_ttl.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ def test_document_ttl_with_positive_defaultTtl(self):

time.sleep(5)

# the created document should NOT be gone as it's ttl value is set to -1(never expire) which overrides the collections's defaultTtl value
# the created document should NOT be gone as its ttl value is set to -1(never expire) which overrides the collection's defaultTtl value
read_document = created_collection.read_item(item=document_definition['id'], partition_key=document_definition['id'])
self.assertEqual(created_document['id'], read_document['id'])

Expand All @@ -166,7 +166,7 @@ def test_document_ttl_with_positive_defaultTtl(self):

time.sleep(4)

# the created document should be gone now as it's ttl value is set to 2 which overrides the collections's defaultTtl value(5)
# the created document should be gone now as its ttl value is set to 2 which overrides the collection's defaultTtl value(5)
self.__AssertHTTPFailureWithStatus(
StatusCodes.NOT_FOUND,
created_collection.read_item,
Expand All @@ -180,7 +180,7 @@ def test_document_ttl_with_positive_defaultTtl(self):

time.sleep(6)

# the created document should NOT be gone as it's ttl value is set to 8 which overrides the collections's defaultTtl value(5)
# the created document should NOT be gone as its ttl value is set to 8 which overrides the collection's defaultTtl value(5)
read_document = created_collection.read_item(item=created_document['id'], partition_key=created_document['id'])
self.assertEqual(created_document['id'], read_document['id'])

Expand Down Expand Up @@ -221,7 +221,7 @@ def test_document_ttl_with_negative_one_defaultTtl(self):

time.sleep(4)

# the created document should be gone now as it's ttl value is set to 2 which overrides the collections's defaultTtl value(-1)
# the created document should be gone now as it's ttl value is set to 2 which overrides the collection's defaultTtl value(-1)
self.__AssertHTTPFailureWithStatus(
StatusCodes.NOT_FOUND,
created_collection.read_item,
Expand Down Expand Up @@ -294,7 +294,7 @@ def test_document_ttl_misc(self):

time.sleep(7)

# Upserted document still exists after 10 secs from document creation time(with collection's defaultTtl set to 8) since it's ttl was reset after 3 secs by upserting it
# Upserted document still exists after 10 secs from document creation time(with collection's defaultTtl set to 8) since its ttl was reset after 3 secs by upserting it
read_document = created_collection.read_item(item=upserted_docment['id'], partition_key=upserted_docment['id'])
self.assertEqual(upserted_docment['id'], read_document['id'])

Expand Down