Skip to content

fix: catch error within customerIntent endpoints #1226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Sep 23, 2025
Merged

fix: catch error within customerIntent endpoints #1226

merged 7 commits into from
Sep 23, 2025

Conversation

@JayKid
Copy link
Contributor

@JayKid JayKid commented Sep 5, 2025 •
edited
Loading

Description

Return status code instead of throwing the whole handler

The PR basically wraps the CRUD customerIntent endpoints in a try catch and returns appropriate status codes instead of allowing the hasAccess function throwing an error to crash the handler.

Please ensure your pull request adheres to the following guidelines:

  • make sure to link the related issues in this description. Or if there's no issue created, make sure you
    describe here the problem you're solving.
  • when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

Related Issues

@JayKid
fix: catch error within customerIntent endpoints
b30c3d6 
Return status code instead of throwing the whole handler
@JayKid
fix: tests
ca0f211
@github-actions
Copy link

github-actions bot commented Sep 5, 2025

This PR will trigger a patch release when merged.

@ravverma
Copy link
Contributor

ravverma commented Sep 8, 2025

@JayKid
Rather having try/catch everywhere , I would handle the error from AccessControlUtil at one place , where it is calling the hasAccess in method getSiteAndValidateLlmo and throw forbidden with message.
https://github.com/adobe/spacecat-api-service/pull/1226/files#diff-98f4e781fc94ce98289e072f10ebfb77c9028d87b18bfdf3d455fc232f8065c0R42

ravverma
ravverma requested changes Sep 8, 2025
View reviewed changes
src/controllers/llmo/llmo.js
const { llmoConfig } = await getSiteAndValidateLlmo(context);
return ok(llmoConfig.customerIntent || []);
try {
const { llmoConfig } = await getSiteAndValidateLlmo(context);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handle it in getSiteAndValidateLlmo method itself rather everyplace

@JayKid
Copy link
Contributor Author

JayKid commented Sep 11, 2025

@JayKid Rather having try/catch everywhere , I would handle the error from AccessControlUtil at one place , where it is calling the hasAccess in method getSiteAndValidateLlmo and throw forbidden with message. https://github.com/adobe/spacecat-api-service/pull/1226/files#diff-98f4e781fc94ce98289e072f10ebfb77c9028d87b18bfdf3d455fc232f8065c0R42

shouldn't be the responsibility of the endpoint to choose what to return to the client, instead of doing it inside of a access helper? or did I misunderstand what you mean by:

throw forbidden with message?

@ravverma
Copy link
Contributor

@JayKid Rather having try/catch everywhere , I would handle the error from AccessControlUtil at one place , where it is calling the hasAccess in method getSiteAndValidateLlmo and throw forbidden with message. https://github.com/adobe/spacecat-api-service/pull/1226/files#diff-98f4e781fc94ce98289e072f10ebfb77c9028d87b18bfdf3d455fc232f8065c0R42

shouldn't be the responsibility of the endpoint to choose what to return to the client, instead of doing it inside of a access helper? or did I misunderstand what you mean by:

throw forbidden with message?

I was just saying that changes are wrapping getSiteAndValidateLlmo in try/catch for error forbidden so better hasAccess wrapped in getSiteAndValidateLlmo once only.

@JayKid
fix: surround with try/catch and return boolean
b216921 
hasAccess should return a boolean (false) in this case and not throw
@JayKid
Copy link
Contributor Author

JayKid commented Sep 16, 2025

@JayKid Rather having try/catch everywhere , I would handle the error from AccessControlUtil at one place , where it is calling the hasAccess in method getSiteAndValidateLlmo and throw forbidden with message. https://github.com/adobe/spacecat-api-service/pull/1226/files#diff-98f4e781fc94ce98289e072f10ebfb77c9028d87b18bfdf3d455fc232f8065c0R42

shouldn't be the responsibility of the endpoint to choose what to return to the client, instead of doing it inside of a access helper? or did I misunderstand what you mean by:

throw forbidden with message?

I was just saying that changes are wrapping getSiteAndValidateLlmo in try/catch for error forbidden so better hasAccess wrapped in getSiteAndValidateLlmo once only.

The problem is that it involves as many changes if not more to change how getSiteAndValidateLlmo works, because all the consumers expect it to throw currently, so I'd need to adapt all consumers if I do that...

@JayKid
fix: tests
fe0a907
@JayKid
Merge branch 'main' into fix-return-403-customer-intent-endpoint
f73c320 
* main: (22 commits)
  Revert "fix: updating lorem-ipsum check name in preflight" (#1261)
  chore(deps): update dependency @adobe/helix-universal to v5.2.3 (#1260)
  chore(release): 1.177.15 [skip ci]
  fix(deps): update adobe fixes (#1258)
  chore(release): 1.177.14 [skip ci]
  fix: update package-lock.json for new tier client (#1257)
  fix: Tier client adapt and remove IDPs from AuthZ (#1256)
  chore(release): 1.177.13 [skip ci]
  fix(deps): update adobe fixes (#1254)
  chore(release): 1.177.12 [skip ci]
  fix(deps): update adobe fixes (#1249)
  chore(release): 1.177.11 [skip ci]
  fix(deps): update dependency jsdom to v27 (#1251)
  chore(release): 1.177.10 [skip ci]
  fix: dont cache empty result (#1252)
  chore(release): 1.177.9 [skip ci]
  fix(deps): update external fixes (#1248)
  chore(release): 1.177.8 [skip ci]
  fix(deps): update dependency @adobe/spacecat-shared-rum-api-client to v2.37.4 (#1247)
  chore(release): 1.177.7 [skip ci]
  ...
@JayKid
Merge branch 'main' into fix-return-403-customer-intent-endpoint
e8fc783 
* main: (34 commits)
  chore(release): 1.182.1 [skip ci]
  fix(deps): update adobe fixes (#1282)
  chore(release): 1.182.0 [skip ci]
  feat: Add tier config option to ASO onboarding command (#1270)
  fix(deps): update dependency @adobe/spacecat-shared-tier-client to v1.1.0 (#1281)
  chore(release): 1.181.0 [skip ci]
  feat: Add key word support for audit command (#1275)
  chore(release): 1.180.2 [skip ci]
  fix(deps): update adobe fixes (#1280)
  chore(release): 1.180.1 [skip ci]
  fix(deps): update external fixes (#1279)
  chore(release): 1.180.0 [skip ci]
  feat: new prerender audit type (#1253)
  chore(release): 1.179.4 [skip ci]
  fix(deps): update dependency @adobe/spacecat-shared-data-access to v2.64.0 (#1277)
  chore(release): 1.179.3 [skip ci]
  fix(deps): update dependency @adobe/spacecat-shared-data-access to v2.63.0 (#1276)
  chore(release): 1.179.2 [skip ci]
  fix(deps): update dependency @adobe/spacecat-shared-utils to v1.50.7 (#1274)
  chore(release): 1.179.1 [skip ci]
  ...
@JayKid JayKid merged commit 0681e62 into main Sep 23, 2025
2 checks passed
@JayKid JayKid deleted the fix-return-403-customer-intent-endpoint branch September 23, 2025 10:06
solaris007 pushed a commit that referenced this pull request Sep 23, 2025
@semantic-release-bot
chore(release): 1.182.2 [skip ci]
1eb57a5 
## [1.182.2](v1.182.1...v1.182.2) (2025-09-23)

### Bug Fixes

* catch error within customerIntent endpoints ([#1226](#1226)) ([0681e62](0681e62))
* llmo controller tests ([#1288](#1288)) ([ce88d60](ce88d60))
@solaris007
Copy link
Member

🎉 This PR is included in version 1.182.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@calvarezg calvarezg calvarezg approved these changes

@ravverma ravverma ravverma approved these changes

Assignees

No one assigned

Labels

released

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@JayKid @ravverma @solaris007 @calvarezg