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

zfoster · 2020-12-08T22:13:07Z

Replaces hardcoded automaticIdGeneration flag with option in container create_item.

Does anyone familiar with this project know why this was flagged True?

cc @annatisch @Rodrigossz @southpolesteve

annatisch · 2020-12-08T23:33:09Z

sdk/cosmos/azure-cosmos/azure/cosmos/container.py

@@ -462,6 +462,7 @@ def create_item(
        pre_trigger_include=None,  # type: Optional[str]
        post_trigger_include=None,  # type: Optional[str]
        indexing_directive=None,  # type: Optional[Any]
+        disable_automatic_id_generation=None, #type: Optional[bool]


Could this be moved into kwargs?
(The :param label in the docstring would move to :keyword to match.)

Maybe it's true to make it easier for those that don't have an id to use.

@annatisch I see no reason why not, however all of the other similar args follow this pattern

Thanks @zfoster - we already have more positional optional parameters in the method signatures than we would generally advise, as making these parameters keyword only gives us more options for non-breaking changes in the future.
So going forward we should generally add new parameters to kwargs unless there's a very compelling reason not to.

@annatisch do you happen to know why this is the case in python?

https://github.com/Azure/azure-sdk-for-python/blob/master/sdk/cosmos/azure-cosmos/test/test_crud.py#L698

or by chance know who would know? :)

Spark Connector creates an id by default. Maybe it's for consistency.

It's doing the opposite here I think? We have hardcoded to DISABLE ID generation

@zfoster I think that comment is just wrong. We should match whatever the JS SDK does here.

annatisch · 2020-12-13T17:49:04Z

sdk/cosmos/azure-cosmos/azure/cosmos/container.py

@@ -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 disable_automatic_id_generation: Enable automatic id generation if no id present.


The default value is True right? If we want non-breaking behaviour, we will probably need to keep automatic generation as disabled by default. So I think we could do that one of two ways:

Keep the parameter called disable_auto_id_gen, and document the default as being True (and set it as such if not present).

Rename the parameter to enable_auto_id_gen, document the default as being False, and set the request option to the opposite.

@zfoster - on thinking about this a bit more - I think we should use the latter option - as I believe it conveys the behaviour more intuitively. i.e. update the kwarg name to enable_auto_id_generation, and update the docstring to state explicitly that this is False by default.
It would mean also updating the implementation to the opposite:

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

Could we please also add a test (or update an existing test) to make sure that this new flag is tested?

annatisch · 2020-12-13T17:52:02Z

sdk/cosmos/azure-cosmos/azure/cosmos/container.py

@@ -488,7 +489,6 @@ def create_item(
        request_options = build_options(kwargs)
        response_hook = kwargs.pop('response_hook', None)

-        request_options["disableAutomaticIdGeneration"] = True


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)

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

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

request_options["disableAutomaticIdGeneration"] = True

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

if i'm reading it right

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)

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)

zfoster · 2020-12-16T19:23:33Z

@annatisch how does this look now?

zfoster · 2021-01-04T18:25:55Z

@annatisch can you take another look at this?

southpolesteve

Approved. As I understand it, this is keeping the existing behavior and adding an option to disable it.

I get why this if confusing. I think this was just done backwards in the original SDK. If we ever do a 5.0 we should look at flipping the behavior to be in sync with the other Cosmos SDKs.

annatisch · 2021-01-08T17:37:08Z

sdk/cosmos/azure-cosmos/azure/cosmos/container.py

@@ -488,7 +489,7 @@ def create_item(
        request_options = build_options(kwargs)
        response_hook = kwargs.pop('response_hook', None)

-        request_options["disableAutomaticIdGeneration"] = True
+        request_options["enableAutomaticIdGeneration"] = kwargs.pop('enable_automatic_id_generation', False)


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)

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

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

…-for-python into zf/auto-id-gen

zfoster · 2021-01-12T13:42:18Z

@annatisch I think this is all set and good for another review

annatisch · 2021-01-12T17:48:45Z

sdk/cosmos/azure-cosmos/azure/cosmos/_cosmos_client_connection.py

        collection_id, document, path = self._GetContainerIdWithPathForItem(
            database_or_container_link, document, options
        )
+
+        if base.IsItemContainerLink(database_or_container_link):
+            print('checking link', document)


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

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

annatisch · 2021-01-12T17:50:35Z

sdk/cosmos/azure-cosmos/azure/cosmos/container.py

@@ -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"}


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

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!

annatisch · 2021-01-15T17:26:12Z

/azp run python - cosmos - tests

azure-pipelines · 2021-01-15T17:26:23Z

Azure Pipelines successfully started running 1 pipeline(s).

…zure#15715) * Add option for disabling auto id generation in create_item * Adds ID generation to kwargs, removes bad test * Default disableAutoID gen to true, add back test * use correct kwarg * Fix test typos * Change flag to enable instead of disable * Adds test for item create with no ID * disable header flag * Fixes auto ID generation in create_item * Remove print and unnecessary passthrough_args

track2 modify msi readme.go.md (Azure#15715)

Add option for disabling auto id generation in create_item

cf925cb

zfoster requested a review from southpolesteve as a code owner December 8, 2020 22:13

ghost added the Cosmos label Dec 8, 2020

annatisch reviewed Dec 8, 2020

View reviewed changes

zfoster mentioned this pull request Dec 9, 2020

create_item overrides any setting by the user #15569

Closed

Adds ID generation to kwargs, removes bad test

937171f

zfoster requested a review from annatisch December 11, 2020 23:32

annatisch requested changes Dec 13, 2020

View reviewed changes

zfoster added 3 commits December 14, 2020 13:45

Default disableAutoID gen to true, add back test

56f9b58

use correct kwarg

4028dd0

Fix test typos

fa3a9fe

southpolesteve approved these changes Jan 7, 2021

View reviewed changes

zfoster added 2 commits January 8, 2021 11:57

Change flag to enable instead of disable

262e672

Adds test for item create with no ID

c6ca91c

annatisch reviewed Jan 8, 2021

View reviewed changes

zfoster and others added 3 commits January 8, 2021 14:05

disable header flag

c12d19d

Merge branch 'zf/auto-id-gen' of https://github.com/zfoster/azure-sdk…

0c99eaa

…-for-python into zf/auto-id-gen

Fixes auto ID generation in create_item

1070de4

zfoster requested review from amishra-dev, ankitarorabit, changlong-liu, chlowell, danieljurek, iscai-msft, johanste, jsntcy, kasobol-msft and KieranBrantnerMagee as code owners January 11, 2021 17:22

zfoster requested review from mccoyp, memontic-ms, mitchdenny, msyyc, rakshith91, RezaJooyandeh, scbedd, schaabs, tasherif-msft, turalf, weshaggard, xiafu-msft, xiangyan99, yunhaoling, zezha-msft and a team as code owners January 11, 2021 17:22

Merge branch 'master' into zf/auto-id-gen

d8c763f

zfoster requested a review from annatisch January 11, 2021 17:30

annatisch reviewed Jan 12, 2021

View reviewed changes

Remove print and unnecessary passthrough_args

9e8a247

rakshith91 removed their request for review January 15, 2021 03:56

zfoster enabled auto-merge (squash) January 15, 2021 17:17

annatisch approved these changes Jan 15, 2021

View reviewed changes

zfoster merged commit 72afdd0 into Azure:master Jan 15, 2021

openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-python that referenced this pull request Aug 20, 2021

CodeGen from PR 15715 in Azure/azure-rest-api-specs

614b951

track2 modify msi readme.go.md (Azure#15715)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

zfoster commented Dec 8, 2020

annatisch Dec 8, 2020 •

edited

Loading

Rodrigossz Dec 8, 2020

zfoster Dec 9, 2020

annatisch Dec 9, 2020

zfoster Dec 9, 2020

zfoster Dec 9, 2020

Rodrigossz Dec 9, 2020

zfoster Dec 9, 2020

southpolesteve Dec 10, 2020

annatisch Dec 13, 2020

annatisch Jan 7, 2021

annatisch Dec 13, 2020

zfoster Dec 14, 2020

annatisch Jan 4, 2021

zfoster Jan 4, 2021

zfoster Jan 4, 2021

annatisch Jan 4, 2021

zfoster Jan 4, 2021

zfoster commented Dec 16, 2020

zfoster commented Jan 4, 2021

southpolesteve left a comment

annatisch Jan 8, 2021

zfoster Jan 8, 2021

annatisch Jan 8, 2021

zfoster commented Jan 12, 2021

annatisch Jan 12, 2021

zfoster Jan 12, 2021

zfoster Jan 12, 2021

annatisch Jan 12, 2021 •

edited

Loading

zfoster Jan 12, 2021

zfoster Jan 12, 2021

annatisch commented Jan 15, 2021

azure-pipelines bot commented Jan 15, 2021

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

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

Conversation

zfoster commented Dec 8, 2020

annatisch Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfoster commented Dec 16, 2020

zfoster commented Jan 4, 2021

southpolesteve left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zfoster commented Jan 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annatisch Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annatisch commented Jan 15, 2021

azure-pipelines bot commented Jan 15, 2021

annatisch Dec 8, 2020 •

edited

Loading

annatisch Jan 12, 2021 •

edited

Loading