Skip to content
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.

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

Fix filter dump in Data Modeling #1510

Merged
merged 10 commits into from
Nov 15, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@ Changes are grouped as follows
- `Fixed` for any bug fixes.
- `Security` in case of vulnerabilities.

## [7.0.3] - 2023-11-15
### Fixed
- Bug when `cognite.client.data_classes.filter` used with any `data_modeling` endpoint raised a `CogniteAPIError` with
snake_cased properties. This is now fixed.

## [7.0.2] - 2023-11-15
### Fixed
- Missing Scope `DataSet` for `TemplateGroupAcl` and `TemplateInstancesAcl`.
Expand Down
2 changes: 1 addition & 1 deletion cognite/client/_api/assets.py
Original file line number Diff line number Diff line change
Expand Up @@ -874,7 +874,7 @@ def filter(
resource_cls=Asset,
method="POST",
limit=limit,
advanced_filter=filter.dump(camel_case=True) if isinstance(filter, Filter) else filter,
advanced_filter=filter.dump(camel_case_property=True) if isinstance(filter, Filter) else filter,
sort=prepare_filter_sort(sort, AssetSort),
other_params={"aggregatedProperties": aggregated_properties_camel} if aggregated_properties_camel else {},
)
Expand Down
13 changes: 7 additions & 6 deletions cognite/client/_api/data_modeling/spaces.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from cognite.client.data_classes.data_modeling.ids import _load_space_identifier
from cognite.client.data_classes.data_modeling.spaces import Space, SpaceApply, SpaceList
from cognite.client.utils._concurrency import ConcurrencySettings
from cognite.client.utils.useful_types import SequenceNotStr


class SpacesAPI(APIClient):
Expand Down Expand Up @@ -63,18 +64,18 @@ def __iter__(self) -> Iterator[Space]:
return self()

@overload
def retrieve(self, spaces: str) -> Space | None: # type: ignore[overload-overlap]
def retrieve(self, spaces: str) -> Space | None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh nice! ...to get overloads working 👌

...

@overload
def retrieve(self, spaces: Sequence[str]) -> SpaceList:
def retrieve(self, spaces: SequenceNotStr[str]) -> SpaceList:
...

def retrieve(self, spaces: str | Sequence[str]) -> Space | SpaceList | None:
def retrieve(self, spaces: str | SequenceNotStr[str]) -> Space | SpaceList | None:
"""`Retrieve one or more spaces. <https://developer.cognite.com/api#tag/Spaces/operation/bySpaceIdsSpaces>`_

Args:
spaces (str | Sequence[str]): Space ID
spaces (str | SequenceNotStr[str]): Space ID

Returns:
Space | SpaceList | None: Requested space or None if it does not exist.
Expand All @@ -100,11 +101,11 @@ def retrieve(self, spaces: str | Sequence[str]) -> Space | SpaceList | None:
executor=ConcurrencySettings.get_data_modeling_executor(),
)

def delete(self, spaces: str | Sequence[str]) -> list[str]:
def delete(self, spaces: str | SequenceNotStr[str]) -> list[str]:
"""`Delete one or more spaces <https://developer.cognite.com/api#tag/Spaces/operation/deleteSpacesV3>`_

Args:
spaces (str | Sequence[str]): ID or ID list ids of spaces.
spaces (str | SequenceNotStr[str]): ID or ID list ids of spaces.
Returns:
list[str]: The space(s) which has been deleted.
Examples:
Expand Down
2 changes: 1 addition & 1 deletion cognite/client/_api/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ def filter(
resource_cls=Event,
method="POST",
limit=limit,
advanced_filter=filter.dump(camel_case=True) if isinstance(filter, Filter) else filter,
advanced_filter=filter.dump(camel_case_property=True) if isinstance(filter, Filter) else filter,
sort=prepare_filter_sort(sort, EventSort),
)

Expand Down
2 changes: 1 addition & 1 deletion cognite/client/_api/sequences.py
Original file line number Diff line number Diff line change
Expand Up @@ -763,7 +763,7 @@ def filter(
resource_cls=Sequence,
method="POST",
limit=limit,
advanced_filter=filter.dump(camel_case=True) if isinstance(filter, Filter) else filter,
advanced_filter=filter.dump(camel_case_property=True) if isinstance(filter, Filter) else filter,
sort=prepare_filter_sort(sort, SequenceSort),
api_subversion="beta",
)
Expand Down
2 changes: 1 addition & 1 deletion cognite/client/_api/time_series.py
Original file line number Diff line number Diff line change
Expand Up @@ -755,7 +755,7 @@ def filter(
resource_cls=TimeSeries,
method="POST",
limit=limit,
advanced_filter=filter.dump(camel_case=True) if isinstance(filter, Filter) else filter,
advanced_filter=filter.dump(camel_case_property=True) if isinstance(filter, Filter) else filter,
sort=prepare_filter_sort(sort, TimeSeriesSort),
)

Expand Down
4 changes: 2 additions & 2 deletions cognite/client/_api_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -452,7 +452,7 @@ def _list_generator(
body["filter"] = filter
if advanced_filter:
body["advancedFilter"] = (
advanced_filter.dump(camel_case=True)
advanced_filter.dump(camel_case_property=True)
if isinstance(advanced_filter, Filter)
else advanced_filter
)
Expand Down Expand Up @@ -603,7 +603,7 @@ def get_partition(partition: int) -> list[dict[str, Any]]:
}
if advanced_filter:
body["advancedFilter"] = (
advanced_filter.dump(camel_case=True)
advanced_filter.dump(camel_case_property=True)
if isinstance(advanced_filter, Filter)
else advanced_filter
)
Expand Down
2 changes: 1 addition & 1 deletion cognite/client/_version.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
from __future__ import annotations

__version__ = "7.0.2"
__version__ = "7.0.3"
__api_subversion__ = "V20220125"
5 changes: 3 additions & 2 deletions cognite/client/data_classes/data_modeling/ids.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from cognite.client.utils._auxiliary import rename_and_exclude_keys
from cognite.client.utils._identifier import DataModelingIdentifier, DataModelingIdentifierSequence
from cognite.client.utils._text import convert_all_keys_recursive, convert_all_keys_to_snake_case
from cognite.client.utils.useful_types import SequenceNotStr


@dataclass(frozen=True)
Expand Down Expand Up @@ -169,7 +170,7 @@ def version(self) -> str | None:
Id = Union[Tuple[str, str], Tuple[str, str, str], IdLike, VersionedIdLike]


def _load_space_identifier(ids: str | Sequence[str]) -> DataModelingIdentifierSequence:
def _load_space_identifier(ids: str | SequenceNotStr[str]) -> DataModelingIdentifierSequence:
is_sequence = isinstance(ids, Sequence) and not isinstance(ids, str)
spaces = [ids] if isinstance(ids, str) else ids
return DataModelingIdentifierSequence(
Expand All @@ -193,7 +194,7 @@ def create_args(id_: Id) -> tuple[str, str, str | None, Literal["node", "edge"]
return id_[0], id_[1], None, id_type # type: ignore[return-value]
raise ValueError("Instance given as a tuple must have two elements (space, externalId)")
if isinstance(id_, tuple):
return id_[0], id_[1], id_[2] if len(id_) == 3 else None, None
return id_[0], id_[1], (id_[2] if len(id_) == 3 else None), None
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this is more readable or not, you choose 😜

Suggested change
return id_[0], id_[1], (id_[2] if len(id_) == 3 else None), None
return (*id_, None, None)[:4]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we catch this in a test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(nvm i see it's already done 💯)

Copy link
Contributor

@haakonvt haakonvt Nov 15, 2023

Choose a reason for hiding this comment

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

@erlendvollset That change was only for easier readability (not referring to my suggestion but his added parens)

instance_type = None
if is_instance:
instance_type = "node" if isinstance(id_, NodeId) else "edge"
Expand Down
16 changes: 14 additions & 2 deletions cognite/client/data_classes/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,20 @@ def _dump_property(property_: PropertyReference, camel_case: bool) -> list[str]
class Filter(ABC):
_filter_name: str

def dump(self, camel_case: bool = True) -> dict[str, Any]:
return {self._filter_name: self._filter_body(camel_case)}
def dump(self, camel_case_property: bool = False) -> dict[str, Any]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the bug introduced in v7, the default was changed from False to True

"""
Dump the filter to a dictionary.

Args:
camel_case_property (bool): Whether to camel case the property names. Defaults to False. Typically,
when the filter is used in data modeling, the property names should not be changed,
while when used with Assets, Event, Sequences, or Files, the property names should be camel cased.
doctrino marked this conversation as resolved.
Show resolved Hide resolved

Returns:
dict[str, Any]: The filter as a dictionary.

"""
return {self._filter_name: self._filter_body(camel_case_property=camel_case_property)}

@classmethod
def load(cls, filter_: dict[str, Any]) -> Filter:
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
[tool.poetry]
name = "cognite-sdk"

version = "7.0.2"
version = "7.0.3"
description = "Cognite Python SDK"
readme = "README.md"
documentation = "https://cognite-sdk-python.readthedocs-hosted.com"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import cognite.client.data_classes.filters as f
from cognite.client.data_classes._base import EnumProperty
from cognite.client.data_classes.data_modeling import ViewId
from cognite.client.data_classes.filters import Filter
from tests.utils import all_subclasses

Expand Down Expand Up @@ -99,7 +100,7 @@ def load_and_dump_equals_data() -> Iterator[ParameterSet]:
@pytest.mark.parametrize("raw_data", list(load_and_dump_equals_data()))
def test_load_and_dump_equals(raw_data: dict) -> None:
parsed = Filter.load(raw_data)
dumped = parsed.dump(camel_case=False)
dumped = parsed.dump(camel_case_property=False)
assert dumped == raw_data


Expand Down Expand Up @@ -152,10 +153,28 @@ def dump_filter_test_data() -> Iterator[ParameterSet]:
}
yield pytest.param(complex_filter, expected, id="And nested and Or with has data and overlaps")

snake_cased_property = f.And(
f.Range(
property=ViewId("space", "viewExternalId", "v1").as_property_ref("start_time"),
lte="2023-09-16T15:50:05.439",
)
)
expected = {
"and": [
{
"range": {
"property": ("space", "viewExternalId/v1", "start_time"),
"lte": "2023-09-16T15:50:05.439",
}
}
]
}
yield pytest.param(snake_cased_property, expected, id="And range filter with snake cased property")


@pytest.mark.parametrize("user_filter, expected", list(dump_filter_test_data()))
def test_dump_filter(user_filter: Filter, expected: dict) -> None:
actual = user_filter.dump(camel_case=False)
actual = user_filter.dump()

assert actual == expected

Expand All @@ -169,7 +188,7 @@ def test_unknown_filter_type() -> None:
def test_user_given_metadata_keys_are_not_camel_cased(property_cls: type) -> None:
# Bug prior to 6.32.4 would dump user given keys in camelCase
flt = f.Equals(property_cls.metadata_key("key_foo_Bar_baz"), "value_foo Bar_baz") # type: ignore [attr-defined]
dumped = flt.dump(camel_case=True)["equals"]
dumped = flt.dump(camel_case_property=True)["equals"]

# property may contain more (static) values, so we just verify the end:
assert dumped["property"][-2:] == ["metadata", "key_foo_Bar_baz"]
Expand Down