feat(gooddata-sdk): report whether create_or_update created or updated - #1705
Open
vondravl wants to merge 1 commit into
Open
feat(gooddata-sdk): report whether create_or_update created or updated#1705vondravl wants to merge 1 commit into
vondravl wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe SDK adds the public ChangesUpsert outcome reporting
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1705 +/- ##
==========================================
+ Coverage 78.30% 78.49% +0.19%
==========================================
Files 271 271
Lines 18689 18719 +30
==========================================
+ Hits 14634 14694 +60
+ Misses 4055 4025 -30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Every create_or_update* method already performs an existence check and branches on it, then discards the answer and returns None. Callers that need to know -- to log accurately, to count creations, to skip follow-up work -- have to repeat the same GET the SDK just made, because the create/update entity calls are reachable only through the private _entities_api. Return a new UpsertOutcome (CREATED / UPDATED) from all nine of them so the information the SDK already computed is no longer thrown away. Adding a return value to a method previously annotated -> None is backward compatible: every in-repo caller (gooddata-pipelines, gooddata-dbt, gooddata-eval) ignores the result and is unaffected. The outcome is best-effort by nature -- the existence check is not atomic with the write that follows -- which the enum docstring states so it is not mistaken for an authoritative audit record. Assertions on both branches are added to the existing cassette tests, and mock-based unit tests cover what no cassette reaches: filter views, export templates, and the update branch of user data filters. Writing those tests turned up a pre-existing defect: the `id is None` create branches of create_or_update_filter_view, create_or_update_user_data_filter and create_or_update_workspace_setting cannot run at all. Each serializes through a generated model requiring a str id, and passing None fails type validation before any request -- including the PostOptionalId variants, where "optional" means "omit the key" rather than "accept None". That is left unfixed here (separate concern), but is pinned by strict xfail tests and the docstrings no longer claim those paths create anything.
vondravl
force-pushed
the
feat/upsert-outcome
branch
from
August 4, 2026 09:05
54285a1 to
d9fb3ba
Compare
vondravl
marked this pull request as ready for review
August 4, 2026 09:13
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py`:
- Around line 37-39: Document that UpsertOutcome returned by
create_or_update_user, create_or_update_user_group, create_or_update,
create_or_update_workspace_setting, create_or_update_user_data_filter, and
create_or_update_filter_view is best-effort and reports the SDK-selected branch
rather than authoritative resource existence. Apply the same qualification in
packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py lines 37-39 and
103-105; packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py
lines 71-73, 164-170, 1234-1240, and 1444-1450. Update the result documentation
in docs/content/en/latest/administration/organization/create_or_update_jwk.md
lines 21-23,
docs/content/en/latest/data/data-source/create_or_update_data_source.md lines
23-26,
docs/content/en/latest/administration/user-groups/create_or_update_user_group.md
lines 22-25,
docs/content/en/latest/administration/users/create_or_update_user.md lines
22-25, and docs/content/en/latest/workspace/workspaces/create_or_update.md lines
18-21 to state that each result is best-effort.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48de74c1-5569-47e0-91c8-c42851ee5c16
📒 Files selected for processing (16)
docs/content/en/latest/administration/organization/create_or_update_jwk.mddocs/content/en/latest/administration/user-groups/create_or_update_user_group.mddocs/content/en/latest/administration/users/create_or_update_user.mddocs/content/en/latest/data/data-source/create_or_update_data_source.mddocs/content/en/latest/workspace/workspaces/create_or_update.mdpackages/gooddata-sdk/src/gooddata_sdk/__init__.pypackages/gooddata-sdk/src/gooddata_sdk/catalog/data_source/service.pypackages/gooddata-sdk/src/gooddata_sdk/catalog/organization/service.pypackages/gooddata-sdk/src/gooddata_sdk/catalog/types.pypackages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.pypackages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.pypackages/gooddata-sdk/tests/catalog/test_catalog_data_source.pypackages/gooddata-sdk/tests/catalog/test_catalog_organization.pypackages/gooddata-sdk/tests/catalog/test_catalog_user_service.pypackages/gooddata-sdk/tests/catalog/test_catalog_workspace.pypackages/gooddata-sdk/tests/catalog/test_upsert_outcome.py
Comment on lines
37
to
+39
| Returns: | ||
| None | ||
| UpsertOutcome: | ||
| CREATED if the user did not exist yet, UPDATED if it did. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document that UpsertOutcome is best-effort.
The existence check and write are not atomic. Another actor can change the resource between these operations. State that the outcome reports the branch selected by the SDK. Do not describe it as an authoritative existence result.
packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py#L37-L39: Add the best-effort qualification tocreate_or_update_user.packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py#L103-L105: Add the best-effort qualification tocreate_or_update_user_group.packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L71-L73: Add the best-effort qualification tocreate_or_update.packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L164-L170: Add the best-effort qualification tocreate_or_update_workspace_setting.packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L1234-L1240: Add the best-effort qualification tocreate_or_update_user_data_filter.packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L1444-L1450: Add the best-effort qualification tocreate_or_update_filter_view.docs/content/en/latest/administration/organization/create_or_update_jwk.md#L21-L23: State that the JWK result is best-effort.docs/content/en/latest/data/data-source/create_or_update_data_source.md#L23-L26: State that the data-source result is best-effort.docs/content/en/latest/administration/user-groups/create_or_update_user_group.md#L22-L25: State that the user-group result is best-effort.docs/content/en/latest/administration/users/create_or_update_user.md#L22-L25: State that the user result is best-effort.docs/content/en/latest/workspace/workspaces/create_or_update.md#L18-L21: State that the workspace result is best-effort.
📍 Affects 7 files
packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py#L37-L39(this comment)packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py#L103-L105packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L71-L73packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L164-L170packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L1234-L1240packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py#L1444-L1450docs/content/en/latest/administration/organization/create_or_update_jwk.md#L21-L23docs/content/en/latest/data/data-source/create_or_update_data_source.md#L23-L26docs/content/en/latest/administration/user-groups/create_or_update_user_group.md#L22-L25docs/content/en/latest/administration/users/create_or_update_user.md#L22-L25docs/content/en/latest/workspace/workspaces/create_or_update.md#L18-L21
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py` around lines
37 - 39, Document that UpsertOutcome returned by create_or_update_user,
create_or_update_user_group, create_or_update,
create_or_update_workspace_setting, create_or_update_user_data_filter, and
create_or_update_filter_view is best-effort and reports the SDK-selected branch
rather than authoritative resource existence. Apply the same qualification in
packages/gooddata-sdk/src/gooddata_sdk/catalog/user/service.py lines 37-39 and
103-105; packages/gooddata-sdk/src/gooddata_sdk/catalog/workspace/service.py
lines 71-73, 164-170, 1234-1240, and 1444-1450. Update the result documentation
in docs/content/en/latest/administration/organization/create_or_update_jwk.md
lines 21-23,
docs/content/en/latest/data/data-source/create_or_update_data_source.md lines
23-26,
docs/content/en/latest/administration/user-groups/create_or_update_user_group.md
lines 22-25,
docs/content/en/latest/administration/users/create_or_update_user.md lines
22-25, and docs/content/en/latest/workspace/workspaces/create_or_update.md lines
18-21 to state that each result is best-effort.
hkad98
reviewed
Aug 4, 2026
| ValidObjects = dict[str, set[str]] | ||
|
|
||
|
|
||
| class UpsertOutcome(str, Enum): |
Contributor
There was a problem hiding this comment.
Nit: we would like to eventually switch this to StrEnum – we cannot right now because we have a support for Python 3.10 which does not support StrEnum.
Consider adding the following:
__str__ = str.__str__
It should make switch to StrEnum a noop.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
UpsertOutcome(CREATED/UPDATED) and returns it from all ninecreate_or_update*methods:catalog_workspacecreate_or_update,create_or_update_workspace_setting,create_or_update_user_data_filter,create_or_update_filter_viewcatalog_usercreate_or_update_user,create_or_update_user_groupcatalog_organizationcreate_or_update_jwk,create_or_update_export_templatecatalog_data_sourcecreate_or_update_data_sourceWhy
Each of these already does an existence check and branches on it, then discards the answer:
A caller that needs to know which happened — to log accurately, count creations, or skip follow-up work — has to repeat the GET the SDK just made, because
create_entity_*/update_entity_*are reachable only through the private_entities_api. This returns information the SDK has already computed.The concrete case that prompted it: a deploy tool logging
"Workspace %s created successfully"after everycreate_or_update, reporting a creation on every re-upload of an existing workspace. The only public-API fix is a duplicateget_workspace()call.Should this be in the SDK at all?
I genuinely don't know, and I'd rather surface the trade-off than argue one side. Happy to close this if the answer is no — the workaround (an extra GET in the caller) is cheap.
Reasons to take it
gooddata-pipelines,gooddata-dbt,gooddata-eval) ignores the return value, and adding a return to a-> Nonemethod breaks no caller or type checker.Reasons to reject it
Nonedoesn't have.get_*probe is the honest place to pay for that knowledge.For what it's worth on intent: I could find no recorded rationale for
-> Nonehere.Returns: Noneis a blanket docstring template across ~80 catalog methods including plain voids likedelete_workspace; there's no comment or doc note explaining it; and-> Nonedates to the originalNAS-3058implementation rather than a later removal. The closest relative,set_hll_type, documents its idempotency but is silent on the outcome. That's absence of evidence, not evidence of absence — the deliberate-choice reading above is still entirely possible, and you'd know better than the history does.Notes on the implementation
UpsertOutcome(str, Enum)rather thanenum.StrEnum—requires-pythonis>=3.10andStrEnumlanded in 3.11. Matches the existingSortDirection(str, Enum)precedent.bool: for an "or update" operation the bool is the entire return value, soif sdk.catalog_workspace.create_or_update(ws):reads as nonsense. It also leaves room forUNCHANGEDlater.None" path (workspace_setting,user_data_filter,filter_view) returnCREATEDon that branch, but see the pre-existing defect below — that branch cannot currently execute, so the docstrings describe it as unreachable rather than promising a creation.Testing
CREATED/UPDATEDmatches what the server actually did.tests/catalog/test_upsert_outcome.pyadds mock-based unit tests for what no cassette reaches: filter views, export templates, and the update branch of user data filters. These stub only the branch-deciding getter, since the branch is chosen by whether that GET raisesNotFoundException.ruff0.15.1 (the pinned pre-commit version) check + format clean;packages/gooddata-sdk: 498 passed, 2 skipped, 3 xfailed.A pre-existing defect this turned up
Writing those tests showed that the
id is Nonecreate branches cannot run at all — increate_or_update_filter_view,create_or_update_user_data_filter, andcreate_or_update_workspace_setting.Each serializes through a generated model that requires a
strid, and passingNonefails type validation before any request is made:This holds for the
PostOptionalIdvariants too, where "optional" means omit the key, not accept None. It predates this PR — the branches were already unreachable; this change only added areturnto them.I have not fixed it here, as it's a separate concern in the entity models / generated client. It's pinned by strict
xfailtests so that fixing it trips them and prompts a revisit, and the docstrings no longer claim those paths create anything. Happy to split it into its own issue or PR if you'd like.Docs
Five
create_or_update*pages had a hardcodedReturns: Noneand are updated (two different legacy formats in use). The remaining four methods have no docs pages.Summary by CodeRabbit
New Features
UpsertOutcometype withCREATEDandUPDATEDvalues.Documentation
Tests