Repository navigation
Pre projects compatibility - #81
carlosfunk wants to merge 1 commit into
Conversation
…der than projects - The VM UPGRADE shards could not seed a 3.26.17 or 3.26.19 server with this client - Every `create_or_update_*` failed: `_resolve_project` raised on the 404 from `/api/projects/` - `_resolve_project` now returns `None` on that 404, so the name lookup sends no `project` filter - `RunInfo.project` and `UnfinishedRun.project` are `None` when the server does not return `project` - A server with projects behaves as before
8ba62c5 to
d67575d
Compare
|
|
||
| def _get_discovery_config_library_id_by_name( | ||
| self, name: str, project: Union[ProjectId, str] | ||
| self, name: str, project: Optional[Union[ProjectId, str]] |
There was a problem hiding this comment.
When _resolve_project returns None, this lookup matches the name in every namespace.
On 3.26.14 to 3.26.19, discovery config libraries are unique on (namespace, name), and a create from this client goes into the default namespace "".
So if the server has emails only in namespace bank/au, create_or_update_discovery_config_library(DiscoveryConfigLibrary(name="emails", ...)) overwrites that library's YAML.
It should create emails in the default namespace instead.
drop_namespace removes the field from the model, so the filter has to run on the raw items when project is None (line 44):
items = response.json()
if project is None:
# A server without projects keys libraries on `(namespace, name)`,
# and a create from this client goes into the default namespace.
items = [item for item in items if item.get("namespace", "") == ""]
entries = [DiscoveryConfigLibrary.model_validate(item) for item in items]|
|
||
| def _get_ruleset_library_id_by_name(self, name: str, project: Union[ProjectId, str]) -> Optional[RulesetLibraryId]: | ||
| def _get_ruleset_library_id_by_name( | ||
| self, name: str, project: Optional[Union[ProjectId, str]] |
There was a problem hiding this comment.
Same problem as in discovery_config_libraries.py.
Here it can also go wrong when a default-namespace library exists:
on 3.26.05 to 3.26.19 ruleset libraries are unique on (namespace, name) and this list is ordered by name only, so limit: 1 can return a library from any namespace with that name.
Keep limit. Without it the server returns a plain list (PAGE_SIZE is None), and Page validation fails.
Instead, order by namespace so that "" comes first, and drop a namespaced result:
params = {"name_exact": name, "project": project, "limit": 1}
if project is None:
# A server without projects keys libraries on `(namespace, name)`.
# Ordering by namespace puts the default namespace first.
params["ordering"] = "namespace"
response = self.make_request("GET", "/api/ruleset-libraries/", params=params)
page_payload = response.json()
if project is None:
page_payload["results"] = [item for item in page_payload["results"] if item.get("namespace", "") == ""]
page = Page[RulesetLibrary].model_validate(page_payload)| The last is the project the server puts a create into when it is sent no `project`. | ||
| With no active project, or more than one, the server rejects such a create, | ||
| so this raises `DataMasqueException` instead. | ||
| Returns `None` for a server older than projects, which has one set of names and no project filter. |
There was a problem hiding this comment.
"which has one set of names" is not true for ruleset and discovery config libraries, which are unique per namespace before 3.26.20 (ruleset libraries from 3.26.05, discovery config libraries from 3.26.14).
Suggest: "Returns None when the server has no /api/projects/, so a lookup sends no project filter."
| self._raise_for_status(response) | ||
|
|
||
| def _resolve_project(self, project: Optional[Union[ProjectId, str]]) -> Union[ProjectId, str]: | ||
| def _resolve_project(self, project: Optional[Union[ProjectId, str]]) -> Optional[Union[ProjectId, str]]: |
There was a problem hiding this comment.
This still fails on 3.26.20 to 3.26.22, the current release.
Those versions have /api/projects/, but their ProjectSerializer returns ["id", "name", "description", "created", "updated"] with no reference.
Project.reference is required, so this line raises a pydantic ValidationError, and every create_or_update_* with no project and no default_project fails.
This function only needs count and the first id, so read those from the JSON:
page_payload = response.json()
active_project_count = page_payload["count"]
if active_project_count != 1:
raise DataMasqueException(
f"No `project` is set and the server has {active_project_count} active projects. Set `project` to choose one."
)
return ProjectId(page_payload["results"][0]["id"])Maybe test with a projects page that has no reference would cover it?
| * ``list_connections``, ``list_table_references`` and ``list_files_of_type`` take an optional ``project`` filter. | ||
| * ``DataMasqueFile`` gains a read-only ``project`` field, which is ``None`` for built-in seeds. | ||
| * ``get_generated_rulesets`` sets each ruleset's ``project`` to the connection's project. | ||
| * ``create_or_update_*`` works with a server older than projects, and ``RunInfo.project`` and ``UnfinishedRun.project`` |
There was a problem hiding this comment.
This holds only when neither the resource's project nor default_project is set.
RulesetSerializer rejects unknown fields ("Got unknown fields") on every release from 2.17.0 to 3.26.19, and RuleLibrarySerializer does on 3.26.05 to 3.26.19.
So with either set, create_or_update_ruleset and create_or_update_ruleset_library send project and get a 400.
Consider saying that here or editing behaviour?
| @@ -41,36 +42,41 @@ def create_connection(client: DataMasqueClient, project: Optional[str], m: reque | |||
|
|
|||
|
|
|||
| def create_discovery_config(client: DataMasqueClient, project: Optional[str], m: requests_mock.Mocker) -> Any: | |||
There was a problem hiding this comment.
These five helpers now call create_or_update_* instead of create_*.
The three existing tests that use CREATES were written for the direct creates.
They still go through create_*, because create_or_update_* calls it.
But if _default_project_field moved into create_or_update_*, create_discovery_config would stop sending default_project and these tests would still pass.
Maybe reverting the five helpers to create_* and giving the new test its own parametrised list of create_or_update_* helpers.
Those helpers can leave project out of the mocked responses, since an old server doesn't return it.
| from tests.helpers import active_projects_page, database_connection_config | ||
|
|
||
| BASE_URL = "http://test-server" | ||
| EMPTY_PAGE = {"count": 0, "next": None, "previous": None, "results": []} |
There was a problem hiding this comment.
tests/test_projects.py:307 already defines EMPTY_PAGE.
Consider moving it to tests/helpers.py and import it in both files.
|
|
||
| response = self.make_request("GET", "/api/projects/", params={"limit": 1}) | ||
| response = self.make_request("GET", "/api/projects/", params={"limit": 1}, require_status_check=False) | ||
| if response.status_code == 404: |
There was a problem hiding this comment.
Nit: add a test that a status other than 404 (for example 403) still raises DataMasqueApiError.
This branch replaces the status check in make_request, and no test checks that only a 404 is treated as a server without projects.
| destination_connection: Optional[RunConnectionRef] = None | ||
| project: str | ||
| """Its project's `<id>-<slug>` reference, e.g. `1-default`.""" | ||
| project: Optional[str] = None |
There was a problem hiding this comment.
Nit: only RunInfo has a test for a payload without project.
Add one for UnfinishedRun, for example through get_unfinished_runs.
No description provided.