refactor(gooddata-api-client): split LDM object permissions per family - #1791
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (47)
🚧 Files skipped from review as they are similar to previous changes (13)
Included review availability: 2 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughThe OpenAPI schemas and generated Python client now use separate attribute, fact, and label permission models. Fact and label management endpoints accept dedicated request models. Obsolete generic LDM object permission references and dashboard permission variants were removed. ChangesPermission model contracts
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The resource-specific permission contracts are consistently propagated through the generated client, with no actionable merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 56.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 102 functions across 24 files. (23 skipped: 22 unsupported, 1 too large.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
A rabbit checks each permission gate Comment |
jeskepetr
marked this pull request as ready for review
September 9, 2026 13:48
metadata-api splits its shared LDM object permission DTOs into one family per grantable resource type (gdc-nas #26771), so the four LdmObjectPermissions* schemas become twelve Attribute/Label/Fact ones. The attribute, label and fact endpoints no longer share request and response models. schemas/gooddata-metadata-client.json is patched rather than downloaded: no backend serves the new shape yet. The patch deletes the four schemas, inserts the twelve verbatim from metadata-api's open-api-spec.json, and repoints the refs in the six operations -- the only places the deleted schemas were referenced. Everything else is `make api-client-local` output, generated with jq 1.7 so that number literals are preserved. One collateral rename: DeclarativeAnalyticalDashboardPermissionForAssigneeRuleAllOf is gone. The generator materialises each inline allOf member as a class, reuses one class across parents whose member is identical, and names it after the first such parent in the alphabetically sorted schema map. AttributePermissionsForAssigneeRule now sorts ahead of the dashboard schema, so the shared class is AttributePermissionsForAssigneeRuleAllOf -- same shape, and the dashboard model still composes it. The dashboard and metric permission models change by that one import only. Depends on gdc-nas #26771 and must not merge before it. JIRA: F1-2851 risk: low Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jeskepetr
force-pushed
the
PJE/F1-2851
branch
from
September 9, 2026 13:53
cd527f1 to
55e22cd
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. |
Vojtasii
approved these changes
Sep 9, 2026
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1791 +/- ##
=======================================
Coverage 81.86% 81.86%
=======================================
Files 277 277
Lines 20016 20016
=======================================
Hits 16387 16387
Misses 3629 3629 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Summary
metadata-api splits its shared LDM object permission DTOs into one family per grantable resource type (gdc-nas #26771). The four
LdmObjectPermissions*schemas become twelveAttribute/Label/Factones, so the attribute, label and fact permission endpoints no longer share request and response models.What is hand-written and what is generated
One file was edited by hand:
schemas/gooddata-metadata-client.json(+204 / −18). Three edits — delete the fourLdmObjectPermissions*schemas, insert the twelve new ones verbatim from metadata-api'sopen-api-spec.json, repoint the$refs in the six attribute/label/fact permission operations.It is patched rather than downloaded because no backend serves the new shape yet, and a full
make downloadwould also sweep in unrelated drift — the checked-in copy predates thecomputedAttributespermission endpoints, among others.Everything else (56 paths:
schemas/gooddata-api-client.json,gooddata-api-client/**) ismake api-client-localoutput with no manual editing. Regenerating should reproduce it byte for byte providedjqis 1.7+ — jq 1.6 does not preserve number literals and rewrites unrelated values across the merged schema.Breaking changes for client users
Removed models:
LdmObjectPermissions,LdmObjectPermissionsAssignment,LdmObjectPermissionsForAssignee,LdmObjectPermissionsForAssigneeAllOf,LdmObjectPermissionsForAssigneeRule.Added: 16 model classes across
Attribute*/Label*/Fact*, includingManageLabelPermissionsRequestInnerandManageFactPermissionsRequestInner— previously all three endpoints shared one inner model, soManageAttributePermissionsRequestInner's composition changes too.One collateral rename:
DeclarativeAnalyticalDashboardPermissionForAssigneeRuleAllOfis gone, even though this PR does not touch dashboards. The generator materialises each inlineallOfmember as a class, reuses one class across parents whose member is identical, and names it after the first such parent in the alphabetically sorted schema map.AttributePermissionsForAssigneeRulenow sorts ahead of the dashboard schema, so the shared class isAttributePermissionsForAssigneeRuleAllOf. The class body is identical apart from the name, and the dashboard model still composes it. The same mechanism already applied before this PR, withPermissionsForAssigneecomposingLdmObjectPermissionsForAssigneeAllOf.Test Plan
make api-client-localcompletes clean; the resulting diff is confined to the permission schemas, their models and docs.ActionsApiandPermissionsApi, now typedAttributePermissions/LabelPermissions/FactPermissions.import gooddata_api_client.modelsloads all 1340 classes.packages/referenced the removed names, andruffexcludesgooddata-api-client.JIRA
F1-2851, epic GDP-3402.
Risk
low — generated-client rename following a backend rename. No behaviour change; the break is limited to imports of the removed class names.
Summary by CodeRabbit
New Features
Documentation
Refactor