Backend frontend update s3 widget params#1779
Open
Artuomka wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThe PR refactors S3 widget credential handling from secret-name references to direct credential values. The interface change propagates from backend data structures through credential-resolution use cases, validation logic, and backend tests to frontend components and their test fixtures. ChangesS3 Widget Credential Parameter Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes This is a straightforward interface contract change applied consistently across backend and frontend. All alterations follow the same pattern: replacing two secret-name-based fields with two direct credential fields. The change is homogeneous (repetitive application of the same refactor across multiple files) with clear semantic intent and no behavioral complexity beyond credential field name substitution. Validation, tests, and documentation are updated in parallel. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 14 UNAVAILABLE: read ECONNRESET Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (1)
backend/test/ava-tests/non-saas-tests/non-saas-s3-widget-e2e.test.ts (1)
721-749: ⚡ Quick winAdd a regression test for legacy widget param keys.
Current coverage checks the new keys only. Please add one case with
access_key_id_secret_name/secret_access_key_secret_nameto lock expected migration behavior and prevent silent breakage.🤖 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 `@backend/test/ava-tests/non-saas-tests/non-saas-s3-widget-e2e.test.ts` around lines 721 - 749, Add a regression test that verifies legacy S3 widget param keys are accepted: create a new test (e.g., test.serial(`${currentTest} - widget creation accepts legacy S3 params`)) in the same file using createConnectionAndTableWithS3Field(), build widget_params JSON with access_key_id_secret_name and secret_access_key_secret_name (instead of the newer keys), POST to /widget/${connectionId}?tableName=${testTableName} like the existing tests, and assert the widget creation is accepted (expect the same success status used by other creation tests, e.g., 201) to ensure migration behavior remains supported.
🤖 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 `@backend/src/entities/s3-widget/use-cases/get-s3-file-url.use.case.ts`:
- Around line 83-90: The code currently always treats params.access_key_id and
params.access_key as secret slugs when calling
userSecretRepository.findSecretBySlugAndCompanyId; instead, detect legacy/direct
credentials first and only call findSecretBySlugAndCompanyId when the value is a
slug/secret reference. Update the logic around accessKeySecret and
secretKeySecret to: 1) check params.access_key_id and params.access_key for
legacy/direct patterns (e.g., AWS key format or non-slug indicator) and use them
directly as credentials if they match, otherwise 2) call
findSecretBySlugAndCompanyId(params.access_key_id, user.company.id) and
findSecretBySlugAndCompanyId(params.access_key, user.company.id) to resolve
secrets; ensure the resulting credential source (direct vs secret) is clearly
documented in the surrounding get-s3-file-url.use.case.ts flow and used
consistently downstream.
---
Nitpick comments:
In `@backend/test/ava-tests/non-saas-tests/non-saas-s3-widget-e2e.test.ts`:
- Around line 721-749: Add a regression test that verifies legacy S3 widget
param keys are accepted: create a new test (e.g., test.serial(`${currentTest} -
widget creation accepts legacy S3 params`)) in the same file using
createConnectionAndTableWithS3Field(), build widget_params JSON with
access_key_id_secret_name and secret_access_key_secret_name (instead of the
newer keys), POST to /widget/${connectionId}?tableName=${testTableName} like the
existing tests, and assert the widget creation is accepted (expect the same
success status used by other creation tests, e.g., 201) to ensure migration
behavior remains supported.
🪄 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: d73460bf-7619-4daf-b6fc-df87d5439ffd
📒 Files selected for processing (13)
backend/src/entities/s3-widget/application/data-structures/s3-widget-params.ds.tsbackend/src/entities/s3-widget/use-cases/get-s3-file-url.use.case.tsbackend/src/entities/s3-widget/use-cases/get-s3-upload-url.use.case.tsbackend/src/entities/widget/utils/validate-create-widgets-ds.tsbackend/test/ava-tests/non-saas-tests/non-saas-s3-widget-e2e.test.tsfrontend/src/app/components/dashboard/db-table-view/db-table-widgets/db-table-widgets.component.tsfrontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.spec.tsfrontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.tsfrontend/src/app/components/ui-components/record-view-fields/s3/s3.component.spec.tsfrontend/src/app/components/ui-components/record-view-fields/s3/s3.component.tsfrontend/src/app/components/ui-components/table-display-fields/s3/s3.component.spec.tsfrontend/src/app/components/ui-components/table-display-fields/s3/s3.component.tsfrontend/src/main.ts
Comment on lines
83
to
90
| const accessKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId( | ||
| params.access_key_id_secret_name, | ||
| params.access_key_id, | ||
| user.company.id, | ||
| ); | ||
|
|
||
| const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId( | ||
| params.secret_access_key_secret_name, | ||
| params.access_key, | ||
| user.company.id, |
There was a problem hiding this comment.
Handle legacy keys (and clarify credential source) before secret lookup.
On Line 84 and Line 89, the code now assumes new keys only, but still treats values as secret slugs. This can break existing stored widgets using legacy keys and conflicts with the “direct credentials” contract.
Proposed compatibility patch
- const accessKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId(
- params.access_key_id,
+ const rawParams = params as Record<string, string | undefined>;
+ const accessKeySlug = rawParams.access_key_id ?? rawParams.access_key_id_secret_name;
+ const secretKeySlug = rawParams.access_key ?? rawParams.secret_access_key_secret_name;
+
+ if (!accessKeySlug || !secretKeySlug) {
+ throw new HttpException({ message: 'Bucket credentials params are missing' }, HttpStatus.BAD_REQUEST);
+ }
+
+ const accessKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId(
+ accessKeySlug,
user.company.id,
);
const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId(
- params.access_key,
+ secretKeySlug,
user.company.id,
);🤖 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 `@backend/src/entities/s3-widget/use-cases/get-s3-file-url.use.case.ts` around
lines 83 - 90, The code currently always treats params.access_key_id and
params.access_key as secret slugs when calling
userSecretRepository.findSecretBySlugAndCompanyId; instead, detect legacy/direct
credentials first and only call findSecretBySlugAndCompanyId when the value is a
slug/secret reference. Update the logic around accessKeySecret and
secretKeySecret to: 1) check params.access_key_id and params.access_key for
legacy/direct patterns (e.g., AWS key format or non-slug indicator) and use them
directly as credentials if they match, otherwise 2) call
findSecretBySlugAndCompanyId(params.access_key_id, user.company.id) and
findSecretBySlugAndCompanyId(params.access_key, user.company.id) to resolve
secrets; ensure the resulting credential source (direct vs secret) is clearly
documented in the surrounding get-s3-file-url.use.case.ts flow and used
consistently downstream.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR renames S3 widget credential parameter fields across backend validation/use cases, frontend widget typing/examples, and related tests.
Changes:
- Renamed S3 widget params from
access_key_id_secret_name/secret_access_key_secret_nametoaccess_key_id/access_key. - Updated backend S3 URL generation and widget validation to use the new names.
- Updated frontend examples and tests to match the new parameter names.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
frontend/src/main.ts |
Reorders ngx-markdown named imports. |
frontend/src/app/components/ui-components/table-display-fields/s3/s3.component.ts |
Updates S3 widget params interface. |
frontend/src/app/components/ui-components/table-display-fields/s3/s3.component.spec.ts |
Updates S3 display test params/import order. |
frontend/src/app/components/ui-components/record-view-fields/s3/s3.component.ts |
Updates S3 widget params interface. |
frontend/src/app/components/ui-components/record-view-fields/s3/s3.component.spec.ts |
Updates record-view S3 test params/import order. |
frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.ts |
Updates S3 widget params interface. |
frontend/src/app/components/ui-components/record-edit-fields/s3/s3.component.spec.ts |
Updates record-edit S3 test params. |
frontend/src/app/components/dashboard/db-table-view/db-table-widgets/db-table-widgets.component.ts |
Updates S3 widget configuration comments/example. |
backend/test/ava-tests/non-saas-tests/non-saas-s3-widget-e2e.test.ts |
Updates S3 widget e2e test params. |
backend/src/entities/widget/utils/validate-create-widgets-ds.ts |
Validates new S3 credential parameter names. |
backend/src/entities/s3-widget/use-cases/get-s3-upload-url.use.case.ts |
Uses new params for upload credential lookup. |
backend/src/entities/s3-widget/use-cases/get-s3-file-url.use.case.ts |
Uses new params for file URL credential lookup. |
backend/src/entities/s3-widget/application/data-structures/s3-widget-params.ds.ts |
Updates backend S3 params interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+47
to
+52
| params.access_key_id, | ||
| user.company.id, | ||
| ); | ||
|
|
||
| const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId( | ||
| params.secret_access_key_secret_name, | ||
| params.access_key, |
Comment on lines
+84
to
+89
| params.access_key_id, | ||
| user.company.id, | ||
| ); | ||
|
|
||
| const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId( | ||
| params.secret_access_key_secret_name, | ||
| params.access_key, |
Comment on lines
+296
to
+297
| "access_key_id": "COMPANY-SECRET/bucket-access-key-id", | ||
| "access_key": "COMPANY-SECRET/bucket-access-key" |
Comment on lines
+47
to
+52
| params.access_key_id, | ||
| user.company.id, | ||
| ); | ||
|
|
||
| const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId( | ||
| params.secret_access_key_secret_name, | ||
| params.access_key, |
Comment on lines
+84
to
+89
| params.access_key_id, | ||
| user.company.id, | ||
| ); | ||
|
|
||
| const secretKeySecret = await this._dbContext.userSecretRepository.findSecretBySlugAndCompanyId( | ||
| params.secret_access_key_secret_name, | ||
| params.access_key, |
Comment on lines
+106
to
+110
| if (!widget_params.access_key_id) { | ||
| errors.push(Messages.WIDGET_REQUIRED_PARAMETER_MISSING('access_key_id')); | ||
| } | ||
| if (!widget_params.secret_access_key_secret_name) { | ||
| errors.push(Messages.WIDGET_REQUIRED_PARAMETER_MISSING('secret_access_key_secret_name')); | ||
| if (!widget_params.access_key) { | ||
| errors.push(Messages.WIDGET_REQUIRED_PARAMETER_MISSING('access_key')); |
Comment on lines
+721
to
+727
| test.serial(`${currentTest} - widget creation rejected when access_key_id missing`, async (t) => { | ||
| const { token, connectionId, testTableName } = await createConnectionAndTableWithS3Field(); | ||
|
|
||
| const widgetParams = JSON.stringify({ | ||
| bucket: 'test-bucket', | ||
| region: 'us-east-1', | ||
| secret_access_key_secret_name: 's', | ||
| access_key: 's', |
Comment on lines
+94
to
+95
| access_key_id: 'test-aws-access-key', | ||
| access_key: 'test-aws-secret-key', |
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
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 by CodeRabbit
Release Notes
Refactor
access_key_idandaccess_keyvalues instead of secret-name references, simplifying widget setup.Tests