fix: ACNA-4617 / #258 make api add additive, dedupe duplicate service records, surface JIL errors#260
Conversation
…ductId
`aio console workspace api add` previously treated any HTTP 200 from the
JIL subscribe endpoint as success and dumped the body through `--json`,
including responses like `{ error: [...], errorDetails: [...] }` that
indicate a partial or total failure. Scripts that check exit code saw a
green run while the workspace was never actually subscribed.
Detect a non-empty `error[]` or `errorDetails[]` and throw a CLI error
instead, so `--json` exits non-zero and the JIL message (e.g. "Service
FrameioAPISDK requires selection of a product") is visible to the user.
Also extend `--license-config` profile matching to accept the licenseConfig
`productId`, in addition to `id` and `name`. Users looking at the output
of `aio console api list --json` reasonably try the `productId` they see
under `properties.licenseConfigs[].productId`; matching by it is harmless
when the (id, name, productId) tuple is unambiguous within a service and
unblocks Frame.io-style services where each product has a single profile.
Root cause of the Frame.io subscription failure: getEnabledServicesForOrg returns FrameioAPISDK twice in this org — once as type: 'adobeid' with no licenseConfigs, and once as type: 'entp' with the product profile metadata required for OAuth Server-to-Server. The previous Array.find by code returned whichever the API listed first (the adobeid record), so availableProfiles was null, --license-config was silently dropped, and JIL rejected the subscription with "requires selection of a product". Replace the find-by-code with pickServiceForCode, which prefers the entp record with populated licenseConfigs when a code appears more than once. This command always uses OAuth Server-to-Server credentials, so picking the entp record matches the credential type and aligns with how the existing interactive prompt filters services (servicesToPromptChoices in aio-cli-lib-console). Verified end-to-end against the reporter's org on fresh Stage and Production workspaces: Frame.io now subscribes cleanly and the resulting OAuth Server-to-Server credential carries the frame.s2s.all scope.
…vices JIL's PUT-services endpoint replaces the credential's service list rather than merging into it, so any second call to `aio console workspace api add` silently wiped the services attached by an earlier call. The success message still said "added", but the prior subscriptions (and their credential scopes) were gone, with no deploy-time signal that anything broke. Fetch the credential's current serviceProperties via getServicePropertiesFromWorkspaceWithCredentialType, merge with the requested adds (requested entry wins on sdkCode overlap so users can still update licenseConfigs), and submit the union. If the fetch fails (e.g. a brand-new workspace with no credential yet) treat it as "no existing services" so the first-time path still works. Dedupe the enabled-services list up front via dedupeServicesByCode so the existing-services helper picks the entp record for FrameioAPISDK in fixServiceProperties and preserves the licenseConfig on round-trip. Verified end-to-end against the reporter's org: after three sequential single-service `api add` calls (AdobeIOManagementAPISDK, FrameioAPISDK, AppBuilderDataServicesSDK), the credential ends up subscribed to all three with their scopes intact, instead of just the last one. Closes #258
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
The diff introduces well-scoped, testable helper functions (pickServiceForCode, dedupeServicesByCode, mergeServiceProperties, assertSubscribeSuccess) with thorough unit tests. The logic is clear, the error handling is defensive, and the documentation comments are accurate. One minor concern is the long chain condition in pickServiceForCode that could be slightly more readable, but it is not a blocking issue. Overall this is high-quality, production-ready code.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Per PR reviewer suggestion. Pure readability change — same behavior, the entp+profiles condition is easier to scan.
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is a well-structured, well-tested set of improvements: productId matching, service deduplication for the Frame.io dual-record case, merge of existing services to prevent silent wipe-out, and surface of JIL embedded errors. Test coverage is thorough. The re-raised readability suggestion about hasLicenseConfigs is still applicable but is a minor style issue. One minor concern: the hasLicenseConfigs inline arrow spans a very long line that could be split for readability.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
CI's Jest branch-coverage threshold (100%) was failing at 98.84%. Three uncovered branches: - dedupeServicesByCode: the if (picked) guard was structurally unreachable (pickServiceForCode always finds at least the iterating element via its matches[0] fallback). Dropped the guard, added a comment explaining why. - existingProperties || []: the fallback was defensive against the lib returning a non-array, but getServicePropertiesFromWorkspaceWithCredentialType always returns either an array of services or [] for a missing credential. Dropped the fallback. - assertSubscribeSuccess: defensive d && d.sdkCode / d && d.message branches for malformed JIL responses were uncovered. Added three tests covering errorDetails entries that lack sdkCode, lack message, or are null.
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is well-structured and thoroughly tested. It correctly addresses the Frame.io duplicate-service edge case, adds merge logic to preserve existing subscriptions, and surfaces JIL embedded errors. One previously raised suggestion about extracting the hasLicenseConfigs predicate into an intermediate variable for readability is still present in the code.
🔄 1 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
- eslint --fix on the assertSubscribeSuccess formatter (stylistic/indent)
- Multi-line hasLicenseConfigs per re-raised PR reviewer suggestion
- JSDoc tweaks (Object<K,V> -> {[k:K]:V}, missing @param description)
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured and thoroughly tested diff. The new helper functions (pickServiceForCode, dedupeServicesByCode, mergeServiceProperties, assertSubscribeSuccess) are each small, focused, and accompanied by comprehensive unit tests. The re-raised suggestion about extracting the hasLicenseConfigs predicate is already addressed in the diff — it is extracted as a named arrow function inside pickServiceForCode. No significant issues found.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Per PR review feedback: parseLicenseConfigFlags accepts any sdkCode key, but the downstream loop only iterates --service-code, so a mismatched key (typo or wrong casing like FrameIOAPISDK vs FrameioAPISDK) was silently ignored while the rest of the command exited 0 with partial state. That's the same silent-drop class of bug ACNA-4617 was filed against. Fail fast if any --license-config key isn't in --service-code, listing the requested service codes in the error so casing typos are visible.
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured and thoughtful patch that addresses several real-world bugs: silent service deduplication issues with Frame.io, missing merge of existing subscriptions before PUT, and swallowed JIL errors in 200 responses. The code is clean, well-documented, and test coverage is comprehensive. A few minor issues are worth noting around edge cases and error handling robustness.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
The lib returns [] (not throws) for a missing credential, so a thrown error here is something real (auth, network, server) and is worth surfacing to the operator. Silently proceeding with an empty list could overwrite the workspace's actual services. Per PR review feedback.
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured and well-tested diff. It fixes real production bugs (Frame.io duplicate service records, silent service list replacement, silent JIL embedded errors) with clear helper functions, good JSDoc, and comprehensive unit tests. Both previously-raised suggestions have been incorporated — the warn-level logging is present and the null-entry handling is correct. One minor issue remains in assertSubscribeSuccess regarding the null errorDetail fallback path, and one small concern about the orphan license-config validation placement.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Per PR review feedback. The lib's getServicePropertiesFromWorkspaceWithCredentialType returns [] (not throws) for a workspace without a credential yet, so any thrown error from this call is a real auth/network/server failure. The previous try/catch swallowed those and proceeded with [], which meant JIL's replace-style PUT would wipe the credential's actual services and replace them with just the requested adds — the exact overwrite this merge is supposed to prevent. Remove the catch and let the error propagate. The user sees the failure and can retry instead of getting a silent state corruption.
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured and thoroughly tested diff. It adds productId matching, service deduplication, merge-before-write semantics, and JIL embedded-error detection — all with clear comments and corresponding tests. The previously flagged null-handling issue in assertSubscribeSuccess remains unaddressed.
🔄 1 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
Per re-raised PR reviewer suggestion. Previously a null entry in errorDetails formatted as the literal string "null" via JSON.stringify fallback — accurate but unclear. Explicit handling produces clearer output for the (unlikely) malformed-response case.
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured and thoroughly tested PR that adds productId matching, service deduplication for duplicate sdkCode entries, service merge to prevent credential overwrites, and JIL embedded-error detection. The previous suggestion regarding null handling in assertSubscribeSuccess has been addressed in the new code. The logic is clearly documented and the test coverage is comprehensive.
✅ LGTM! This PR looks good to merge.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
|
@dthampy will wait for your approval |
|
Hey Patrick, sorry for the delay and thanks for taking care of both the issues |
3 participants
Summary
Three related fixes to
aio console workspace api add, all surfaced by a Frame.io API subscription failure in a customer-facing org. Each is independently correct and ships together to leave the command in a consistent state.1. Surface JIL embedded errors as CLI errors
JIL's subscribe endpoint returns 200 with
{ error: [...], errorDetails: [...] }embedded for partial/total failures. The CLI was passing that through--jsonas if it succeeded, so scripts checking exit code silently passed while the subscription never landed. NewassertSubscribeSuccessruns after the subscribe call and throws a CLI error iferror[]orerrorDetails[]is non-empty.2. Dedupe duplicate service records by sdkCode
Some orgs return the same
sdkCodetwice ingetEnabledServicesForOrg— once astype: 'adobeid'(browser/SPA flow, nolicenseConfigs) and once astype: 'entp'(with the product profile metadata required for OAuth Server-to-Server). The previousArray.findby code returned whichever the API listed first; in the repro org that was the adobeid record, so--license-configwas silently dropped and JIL rejected the subscription with "requires selection of a product".New
pickServiceForCodeprefers the entp record with populatedlicenseConfigswhen a code appears more than once, matching how the existing interactive prompt filters services.dedupeServicesByCodeapplies this up front so subsequent code can just.find().Also extended
resolveLicenseConfigsto match byproductId(in addition toidandname), since users looking ataio console api listreasonably try theproductIdthey see underproperties.licenseConfigs[].productId.3. Make
api addadditive instead of overwriting (closes #258)JIL's PUT-services endpoint is set-based — it replaces the credential's service list rather than appending. Any second call to
aio console workspace api addsilently wiped services from the first call, breaking deployed apps that depended on the removed scopes with no deploy-time signal.Fetch the credential's current
servicePropertiesviagetServicePropertiesFromWorkspaceWithCredentialType, merge with the requested adds (requested entry wins on sdkCode overlap so users can still update licenseConfigs), and submit the union. Failure to fetch (e.g. brand-new workspace with no credential yet) is caught and treated as "no existing services" so the first-time path still works.End-to-end verification
Against a fresh test project in the repro org (Forward Deployed Engineering - Firefly Services):
api add --service-code FrameioAPISDK --license-config FrameioAPISDK=875473476against Stage → success, credential carriesframe.s2s.allscopeAdobeIOManagementAPISDK, thenFrameioAPISDK, thenAppBuilderDataServicesSDK→ credential ends up subscribed to all three with their scopes intact (previously the second and third calls would have dropped the prior services)Test plan
add.jsCloses