Skip to content

Commit 0d86ab2

Browse files
thejcfactorthejcfactor
committed
PYCBC-1770: Collection Management create_collection and drop_collection have an overload path that is not setting options
Changes ------- * Fix possible `UnboundLocalError` in `CollectionMgmtRequestBuilder` build_create_collection_request() and build_drop_collection_request() by restructuring argument extraction (positional, keyword and mixed) * Add more test coverage to collection_mgmt_t (acrosss all APIs) modules to confirm we handle the various input param scenarios for create_collection() and drop_collection() Change-Id: I76dc5cd23eaa67f0909a5807425b74e3f90589da Reviewed-on: https://review.couchbase.org/c/couchbase-python-client/+/244935 Reviewed-by: Dimitris Christodoulou <dimitris.christodoulou@couchbase.com> Tested-by: Build Bot <build@couchbase.com>
1 parent fab7f52 commit 0d86ab2

4 files changed

Lines changed: 496 additions & 28 deletions

File tree

‎acouchbase/tests/collectionmgmt_t.py‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
from couchbase.management.collections import (CollectionSpec,
3333
CreateCollectionSettings,
3434
UpdateCollectionSettings)
35+
from couchbase.management.options import CreateCollectionOptions, DropCollectionOptions
3536

3637
from ._test_utils import TestEnvironment
3738

@@ -174,6 +175,83 @@ async def test_create_collection(self, cb_env):
174175
await cb_env.test_bucket_cm.create_collection("_default", self.TEST_COLLECTION)
175176
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
176177

178+
@pytest.mark.usefixtures("cleanup_collection")
179+
@pytest.mark.asyncio
180+
async def test_create_collection_kwargs(self, cb_env):
181+
# all-keyword form
182+
await cb_env.test_bucket_cm.create_collection(scope_name="_default",
183+
collection_name=self.TEST_COLLECTION)
184+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
185+
186+
@pytest.mark.usefixtures("cleanup_collection")
187+
@pytest.mark.asyncio
188+
async def test_create_collection_mixed_kwargs(self, cb_env):
189+
# positional scope_name, keyword collection_name
190+
await cb_env.test_bucket_cm.create_collection("_default",
191+
collection_name=self.TEST_COLLECTION)
192+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
193+
194+
@pytest.mark.usefixtures("cleanup_collection")
195+
@pytest.mark.asyncio
196+
async def test_create_collection_settings_kwarg(self, cb_env):
197+
if cb_env.is_mock_server:
198+
pytest.skip("CAVES doesn't support collection expiry.")
199+
# positional names with settings passed as a keyword
200+
settings = CreateCollectionSettings(max_expiry=timedelta(seconds=2))
201+
await cb_env.test_bucket_cm.create_collection("_default",
202+
self.TEST_COLLECTION,
203+
settings=settings)
204+
coll_spec = await cb_env.get_collection("_default", self.TEST_COLLECTION)
205+
assert coll_spec is not None
206+
assert coll_spec.max_expiry == timedelta(seconds=2)
207+
208+
@pytest.mark.usefixtures("cleanup_collection")
209+
@pytest.mark.asyncio
210+
async def test_create_collection_with_options(self, cb_env):
211+
# positional names + positional options
212+
opts = CreateCollectionOptions(timeout=timedelta(seconds=30))
213+
await cb_env.test_bucket_cm.create_collection("_default", self.TEST_COLLECTION, None, opts)
214+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
215+
216+
@pytest.mark.usefixtures("cleanup_collection")
217+
@pytest.mark.asyncio
218+
async def test_create_collection_timeout_kwarg(self, cb_env):
219+
# kwargs path + timeout as a raw keyword
220+
await cb_env.test_bucket_cm.create_collection(scope_name="_default",
221+
collection_name=self.TEST_COLLECTION,
222+
timeout=timedelta(seconds=30))
223+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
224+
225+
@pytest.mark.usefixtures("cleanup_collection")
226+
@pytest.mark.asyncio
227+
async def test_create_collection_collection_spec_kwarg(self, cb_env):
228+
spec = CollectionSpec(self.TEST_COLLECTION)
229+
await cb_env.test_bucket_cm.create_collection(collection=spec)
230+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
231+
232+
@pytest.mark.asyncio
233+
async def test_create_collection_invalid_args(self, cb_env):
234+
spec = CollectionSpec("coll", "scope")
235+
with pytest.raises(InvalidArgumentException):
236+
await cb_env.test_bucket_cm.create_collection(spec, collection=spec)
237+
with pytest.raises(InvalidArgumentException):
238+
await cb_env.test_bucket_cm.create_collection(spec, scope_name="_default")
239+
with pytest.raises(InvalidArgumentException):
240+
await cb_env.test_bucket_cm.create_collection(spec, settings=CreateCollectionSettings())
241+
with pytest.raises(InvalidArgumentException):
242+
await cb_env.test_bucket_cm.create_collection(collection=spec, collection_name="c")
243+
with pytest.raises(InvalidArgumentException):
244+
await cb_env.test_bucket_cm.create_collection("_default", collection=spec)
245+
with pytest.raises(InvalidArgumentException):
246+
await cb_env.test_bucket_cm.create_collection("_default", scope_name="_default")
247+
with pytest.raises(InvalidArgumentException):
248+
await cb_env.test_bucket_cm.create_collection("_default", "c", collection_name="c")
249+
with pytest.raises(InvalidArgumentException):
250+
await cb_env.test_bucket_cm.create_collection("_default", "c", CreateCollectionSettings(),
251+
settings=CreateCollectionSettings())
252+
with pytest.raises(InvalidArgumentException):
253+
await cb_env.test_bucket_cm.create_collection("_default", "c", settings="not-a-settings")
254+
177255
@pytest.mark.usefixtures("cleanup_scope")
178256
@pytest.mark.asyncio
179257
async def test_create_scope_and_collection(self, cb_env):
@@ -316,6 +394,60 @@ async def test_drop_collection(self, cb_env):
316394
with pytest.raises(CollectionNotFoundException):
317395
await cb_env.test_bucket_cm.drop_collection("_default", self.TEST_COLLECTION)
318396

397+
@pytest.mark.usefixtures("cleanup_collection")
398+
@pytest.mark.asyncio
399+
async def test_drop_collection_kwargs(self, cb_env):
400+
await cb_env.test_bucket_cm.create_collection("_default", self.TEST_COLLECTION)
401+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
402+
await cb_env.test_bucket_cm.drop_collection(scope_name="_default",
403+
collection_name=self.TEST_COLLECTION)
404+
with pytest.raises(CollectionNotFoundException):
405+
await cb_env.test_bucket_cm.drop_collection(scope_name="_default",
406+
collection_name=self.TEST_COLLECTION)
407+
408+
@pytest.mark.usefixtures("cleanup_collection")
409+
@pytest.mark.asyncio
410+
async def test_drop_collection_with_options(self, cb_env):
411+
opts = DropCollectionOptions(timeout=timedelta(seconds=30))
412+
await cb_env.test_bucket_cm.create_collection("_default", self.TEST_COLLECTION)
413+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
414+
await cb_env.test_bucket_cm.drop_collection("_default", self.TEST_COLLECTION, opts)
415+
416+
@pytest.mark.usefixtures("cleanup_collection")
417+
@pytest.mark.asyncio
418+
async def test_drop_collection_timeout_kwarg(self, cb_env):
419+
await cb_env.test_bucket_cm.create_collection("_default", self.TEST_COLLECTION)
420+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
421+
await cb_env.test_bucket_cm.drop_collection(scope_name="_default",
422+
collection_name=self.TEST_COLLECTION,
423+
timeout=timedelta(seconds=30))
424+
425+
@pytest.mark.usefixtures("cleanup_collection")
426+
@pytest.mark.asyncio
427+
async def test_drop_collection_collection_spec_kwarg(self, cb_env):
428+
spec = CollectionSpec(self.TEST_COLLECTION)
429+
await cb_env.test_bucket_cm.create_collection(spec)
430+
assert await cb_env.get_collection("_default", self.TEST_COLLECTION) is not None
431+
await cb_env.test_bucket_cm.drop_collection(collection=spec)
432+
with pytest.raises(CollectionNotFoundException):
433+
await cb_env.test_bucket_cm.drop_collection(collection=spec)
434+
435+
@pytest.mark.asyncio
436+
async def test_drop_collection_invalid_args(self, cb_env):
437+
spec = CollectionSpec("coll", "scope")
438+
with pytest.raises(InvalidArgumentException):
439+
await cb_env.test_bucket_cm.drop_collection(spec, collection=spec)
440+
with pytest.raises(InvalidArgumentException):
441+
await cb_env.test_bucket_cm.drop_collection(spec, scope_name="_default")
442+
with pytest.raises(InvalidArgumentException):
443+
await cb_env.test_bucket_cm.drop_collection(collection=spec, collection_name="c")
444+
with pytest.raises(InvalidArgumentException):
445+
await cb_env.test_bucket_cm.drop_collection("_default", collection=spec)
446+
with pytest.raises(InvalidArgumentException):
447+
await cb_env.test_bucket_cm.drop_collection("_default", scope_name="_default")
448+
with pytest.raises(InvalidArgumentException):
449+
await cb_env.test_bucket_cm.drop_collection("_default", "c", collection_name="c")
450+
319451
@pytest.mark.asyncio
320452
async def test_drop_collection_not_found(self, cb_env):
321453
with pytest.raises(CollectionNotFoundException):

‎couchbase/management/logic/collection_mgmt_req_builder.py‎

Lines changed: 74 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,76 @@ def _get_collection_spec_details(self,
9999
collection_name = collection_spec.name
100100
return scope_name, collection_name, settings
101101

102+
def _extract_collection_overload_args(self, # noqa: C901
103+
method_name: str,
104+
args: Tuple[object, ...],
105+
kwargs: Dict[str, Any]
106+
) -> Tuple[Optional[str],
107+
Optional[str],
108+
Optional[CreateCollectionSettings],
109+
Tuple[object, ...]]:
110+
supports_settings = method_name == 'create_collection'
111+
112+
spec_positional = bool(args) and isinstance(args[0], CollectionSpec)
113+
spec_keyword = isinstance(kwargs.get('collection'), CollectionSpec)
114+
115+
if spec_positional and spec_keyword:
116+
raise InvalidArgumentException(
117+
f"{method_name}: CollectionSpec provided both positionally and as 'collection' keyword.")
118+
119+
if spec_positional or spec_keyword:
120+
conflicting = [k for k in ('scope_name', 'collection_name', 'settings') if k in kwargs]
121+
if conflicting:
122+
raise InvalidArgumentException(
123+
f"{method_name}: cannot mix CollectionSpec form with keyword argument(s) "
124+
f"{', '.join(repr(c) for c in conflicting)}.")
125+
if spec_positional:
126+
collection_spec = args[0]
127+
options = args[1:]
128+
elif args:
129+
raise InvalidArgumentException(
130+
f"{method_name}: cannot mix 'collection' keyword (CollectionSpec) with positional arguments.")
131+
else:
132+
collection_spec = kwargs.pop('collection')
133+
options = ()
134+
scope_name, collection_name, settings = self._get_collection_spec_details(collection_spec, method_name)
135+
if not supports_settings:
136+
settings = None
137+
return scope_name, collection_name, settings, options
138+
139+
pos = list(args)
140+
141+
if pos:
142+
if 'scope_name' in kwargs:
143+
raise InvalidArgumentException(
144+
f"{method_name}: 'scope_name' provided both positionally and as a keyword argument.")
145+
scope_name = pos.pop(0)
146+
else:
147+
scope_name = kwargs.pop('scope_name', None)
148+
149+
if pos:
150+
if 'collection_name' in kwargs:
151+
raise InvalidArgumentException(
152+
f"{method_name}: 'collection_name' provided both positionally and as a keyword argument.")
153+
collection_name = pos.pop(0)
154+
else:
155+
collection_name = kwargs.pop('collection_name', None)
156+
157+
settings = None
158+
if supports_settings:
159+
if pos and (isinstance(pos[0], CreateCollectionSettings) or pos[0] is None):
160+
if 'settings' in kwargs:
161+
raise InvalidArgumentException(
162+
f"{method_name}: 'settings' provided both positionally and as a keyword argument.")
163+
settings = pos.pop(0)
164+
else:
165+
settings = kwargs.pop('settings', None)
166+
if settings is not None and not isinstance(settings, CreateCollectionSettings):
167+
raise InvalidArgumentException(
168+
f"{method_name}: 'settings' must be a CreateCollectionSettings instance.")
169+
170+
return scope_name, collection_name, settings, tuple(pos)
171+
102172
def _validate_bucket_name(self, bucket_name: str) -> None:
103173
if is_null_or_empty(bucket_name):
104174
raise InvalidArgumentException('The bucket_name cannot be empty.')
@@ -126,23 +196,8 @@ def build_create_collection_request(self,
126196
**kwargs: object) -> CreateCollectionRequest:
127197
# the obs_handler is required, let pop fail if not provided
128198
obs_handler: ObservableRequestHandler = kwargs.pop('obs_handler')
129-
settings = None
130-
if args and isinstance(args[0], CollectionSpec):
131-
collection_spec = args[0]
132-
options = args[1:]
133-
scope_name, collection_name, settings = self._get_collection_spec_details(collection_spec,
134-
'create_collection')
135-
elif len(args) >= 2:
136-
scope_name = args[0]
137-
collection_name = args[1]
138-
options_start = 2
139-
if len(args) > options_start and isinstance(args[options_start], CreateCollectionSettings):
140-
settings = args[2]
141-
options_start = 3
142-
options = args[options_start:] if len(args) > options_start else ()
143-
else:
144-
scope_name = None
145-
collection_name = None
199+
scope_name, collection_name, settings, options = self._extract_collection_overload_args(
200+
'create_collection', args, kwargs)
146201

147202
final_args = forward_args(kwargs, *options)
148203
parent_span = ObservableRequestHandler.maybe_get_parent_span(parent_span=final_args.pop('parent_span', None))
@@ -208,17 +263,8 @@ def build_drop_collection_request(self,
208263
**kwargs: object) -> CreateCollectionRequest:
209264
# the obs_handler is required, let pop fail if not provided
210265
obs_handler: ObservableRequestHandler = kwargs.pop('obs_handler')
211-
if args and isinstance(args[0], CollectionSpec):
212-
collection_spec = args[0]
213-
options = args[1:]
214-
scope_name, collection_name, _ = self._get_collection_spec_details(collection_spec, 'drop_collection')
215-
elif len(args) >= 2:
216-
scope_name = args[0]
217-
collection_name = args[1]
218-
options = args[2:]
219-
else:
220-
scope_name = None
221-
collection_name = None
266+
scope_name, collection_name, _, options = self._extract_collection_overload_args(
267+
'drop_collection', args, kwargs)
222268

223269
final_args = forward_args(kwargs, *options)
224270
parent_span = ObservableRequestHandler.maybe_get_parent_span(parent_span=final_args.pop('parent_span', None))

0 commit comments

Comments
 (0)