Skip to content

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

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

Merged
zfoster 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 7 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
3 changes: 2 additions & 1 deletion 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 @@ -475,6 +475,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 @@ -488,7 +489,7 @@ def create_item(
request_options = build_options(kwargs)
response_hook = kwargs.pop('response_hook', None)

request_options["disableAutomaticIdGeneration"] = True

@annatisch annatisch Dec 13, 2020

Copy link
Copy Markdown
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)

@zfoster zfoster Dec 14, 2020

Copy link
Copy Markdown
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

@annatisch annatisch Jan 4, 2021

Copy link
Copy Markdown
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?

@zfoster zfoster Jan 4, 2021

Copy link
Copy Markdown
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

@zfoster zfoster Jan 4, 2021

Copy link
Copy Markdown
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

@annatisch annatisch Jan 4, 2021

Copy link
Copy Markdown
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)

@zfoster zfoster Jan 4, 2021

Copy link
Copy Markdown
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["enableAutomaticIdGeneration"] = kwargs.pop('enable_automatic_id_generation', False)

@annatisch annatisch Jan 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The request_options key here is a service value - not sure it can be changed like this?
I think you'll need something like:

        request_options["disableAutomaticIdGeneration"] = not kwargs.pop('enable_automatic_id_generation', False)

@zfoster zfoster Jan 8, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yeah definitely - I can't push at the moment, having issues authenticating to github. I slapped something together and then was testing pushing. I'll reping this when ready for review

@annatisch annatisch Jan 8, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome, thanks! :)
Good luck with the github trouble - let me know if you need me to make any updates from my end.

if populate_query_metrics:
request_options["populateQueryMetrics"] = populate_query_metrics
if pre_trigger_include is not None:
Expand Down
9 changes: 9 additions & 0 deletions 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 @@ -789,6 +789,15 @@ def test_document_crud(self):
self.assertEqual(created_document.get('id'),
document_definition['id'])

# 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'])

# duplicated documents are not allowed when 'id' is provided.
duplicated_definition_with_id = document_definition.copy()
self.__AssertHTTPFailureWithStatus(StatusCodes.CONFLICT,
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