Make create_query_index always supply partitioned
#470
Conversation
bessbd
force-pushed
the
fix-create_query_index-missing-partitioned
branch
from
56d1a07 to
1a9cd98
Compare
`create_query_index` does not always supply `partitioned` which is always required by the service. This commit changes that so that `partitioned` is now always present in the `create_query_index` requests. This fixes #468
bessbd
force-pushed
the
fix-create_query_index-missing-partitioned
branch
from
1a9cd98 to
588bf00
Compare
| @@ -47,7 +47,7 @@ class Index(object): | |||
| :func:`~cloudant.database.CloudantDatabase.create_query_index`. | |||
| """ | |||
|
|
|||
| def __init__(self, database, design_document_id=None, name=None, partitioned=False, **kwargs): | |||
| def __init__(self, database, design_document_id=None, name=None, partitioned=None, **kwargs): | |||
There was a problem hiding this comment.
I think you'll need to update the docs to match the behaviour.
Similarly, to define a partitioned Cloudant Query index you must set the partitioned=True optional.
I think we didn't actually honour this behaviour before - i.e. we didn't write False and the server side default was true in a partitioned database so it wasn't actually a requirement to set partitioned=True (even though I think that was the intention†).
† - IIRC the intention was to provide a stable default value (False) client-side rather than following the server-side approach where the default changed depending on whether the database is partitioned or not.
There was a problem hiding this comment.
The docs seem correct - as in it looks like it reflects the current behavior.
Would you like me to add that partitioned=False needs to be explicitly set for unpartitioned cases?
There was a problem hiding this comment.
Is this truth table accurate?
| Client-side value | Partitioned Database | Resulting index is partitioned |
|---|---|---|
None |
False |
False |
None |
True |
True |
False |
False |
False |
False |
True |
False |
True |
False |
400 |
True |
True |
True |
If so then doesn't the second row contradict the docs because you can get a partitioned index without setting partitioned=True in one case?
There was a problem hiding this comment.
I was only considering this from a python-cloudant perspective, where a specified partitioned=False was not passed to the service because of https://github.com/cloudant/python-cloudant/pull/470/files#diff-eeec48d80ce666f16517361d09cf63f1L157 , but that is required according to https://docs.couchdb.org/en/stable/api/database/find.html#post--db-_index , so a "third" boolean state is necessary ( https://github.com/cloudant/python-cloudant/pull/470/files#diff-eeec48d80ce666f16517361d09cf63f1R157 ) the "high impedance" None, because some versions of the service return a 400 for a specified partitioned: https://travis-ci.org/github/cloudant/python-cloudant/jobs/686082038#L426 .
If so then doesn't the second row contradict the docs because you can get a partitioned index without setting
partitioned=Truein one case?
My understanding as per https://docs.couchdb.org/en/stable/api/database/find.html#post--db-_index is that partitioned always has to be specified in recent versions of CouchDB.
Do you have any specific doc changes in mind? That should help me understand what I'm missing.
There was a problem hiding this comment.
What I was trying to say is that your change means that in the case where the database is partitioned and the user does not specify a value for partitioned in python-cloudant the resulting index will still be created as partitioned (because of the server-side default).
The documentation states that:
you must set the partitioned=True
But that "must" does not apply in the case described. I understand the confusion because the code didn't actually work as the doc described before (i.e. your change preserves the existing behaviour for this particular case).
The docs statement:
Similarly, to define a partitioned Cloudant Query index you must set the partitioned=True optional.
needs to change to something like:
To define a partitioned Cloudant Query index you may set the
partitioned=Trueoptional, but it is not required as the index will be partitioned by default in a partitioned database. Conversely, you must set thepartitioned=Falseoptional if you wish to create a global (non-partitioned) index in a partitioned database.
CHANGES.md
Outdated
| @@ -1,6 +1,7 @@ | |||
| # UNRELEASED | |||
|
|
|||
| - [FIXED] Set default value for `partitioned` parameter to false when creating a design document. | |||
| - [FIXED] Always set `partitioned` for `create_query_index` requests | |||
There was a problem hiding this comment.
I think this could be a little confusing for a user because it could imply that we're always setting partitioned=True and nit for a trailing period/full stop.
How about:
- [FIXED] - Corrected setting of `partitioned` flag for `create_query_index` requests.
|
Thank you for the review, @ricellis ! I'm about to merge this once CI passes. |
Checklist
CHANGES.md|CHANGELOG.md) or test/build only changesDescription
The issue is described in #468 .
create_query_indexdoes not always supplypartitionedwhich isalways required by the service.
Approach
This PR changes the behavior that so that
partitionedis now alwayspresent in the
create_query_indexrequests.Schema & API Changes
- "No change"EDIT: Two default
Falseparameters have been changed toNone. The users should not notice any difference.Security and Privacy
Testing
Monitoring and Logging
This fixes #468