[Cosmos] Add option for disabling auto id generation in create_item #15715
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
| :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 | ||
|
|
@@ -488,7 +489,6 @@ def create_item( | |
| request_options = build_options(kwargs) | ||
| response_hook = kwargs.pop('response_hook', None) | ||
|
|
||
|
There was a problem hiding this comment. 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)There was a problem hiding this comment. 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 There was a problem hiding this comment. Thanks @zfoster - just to clarify, when you say "requires them to use an ID already", whereabouts is that? There was a problem hiding this comment. having this hardcoded means you have to supply an ID in the args when creating items There was a problem hiding this comment. if i'm reading it right There was a problem hiding this comment. 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) There was a problem hiding this comment. 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) |
||
| if populate_query_metrics: | ||
| request_options["populateQueryMetrics"] = populate_query_metrics | ||
| if pre_trigger_include is not None: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
disable_auto_id_gen, and document the default as beingTrue(and set it as such if not present).enable_auto_id_gen, document the default as beingFalse, and set the request option to the opposite.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 isFalseby default.It would mean also updating the implementation to the opposite:
Could we please also add a test (or update an existing test) to make sure that this new flag is tested?