feat(gooddata-sdk): report whether create_or_update created or updated - #1705
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (16)
🚧 Files skipped from review as they are similar to previous changes (16)
📝 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.40% 78.59% +0.19%
==========================================
Files 271 271
Lines 18741 18772 +31
==========================================
+ Hits 14693 14754 +61
+ Misses 4048 4018 -30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Contributor
Author
There was a problem hiding this comment.
Done in c618ee2 — added __str__ = str.__str__ on UpsertOutcome.
Verified it makes the eventual StrEnum swap a no-op: str(UpsertOutcome.CREATED) / f"{...}" / "%s" % / json.dumps all yield "created" instead of "UpsertOutcome.CREATED", while repr(), == "created" and UpsertOutcome("created") are unchanged. Pinned it in test_upsert_outcome.py::test_outcome_is_a_plain_string (str(outcome) == outcome.value + f-string), so a future base-class change that regresses this trips the test.
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.
str(UpsertOutcome.CREATED) returned "UpsertOutcome.CREATED"; with __str__ = str.__str__ it returns "created", so swapping the base class for StrEnum once py3.10 support is dropped is a no-op for callers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vondravl
force-pushed
the
feat/upsert-outcome
branch
from
August 5, 2026 05:58
c618ee2 to
b8993c4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
hkad98
approved these changes
Aug 5, 2026
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