You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Sameerali0 (via GitHub)" <gi...@apache.org> on 2025/06/29 02:33:09 UTC
[PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Sameerali0 opened a new pull request, #33965:
URL: https://github.com/apache/superset/pull/33965
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
The user was not able to inter commas into the input(schemas allowed for file) because the filter function was removing them so I added some logic to check if it is the last comma then it shouldnt be removed.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [x] Has associated issue: Fixes #28252
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #33965:
URL: https://github.com/apache/superset/pull/33965#discussion_r2200600497
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
Also the failing test indicates there is an issue when the allowed schemas are empty
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
This seems like a good suggestion
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "Sameerali0 (via GitHub)" <gi...@apache.org>.
Sameerali0 commented on PR #33965:
URL: https://github.com/apache/superset/pull/33965#issuecomment-3016238208
@msyavuz
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33965:
URL: https://github.com/apache/superset/pull/33965#discussion_r2222002615
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
Ah, I see - we need to avoid adding empty schemas altogether. Let's modify the filter to:
```typescript
.split(',')
.map(schema => schema.trim())
.filter(schema => schema !== '')
```
This will keep only non-empty schemas after trimming.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "Sameerali0 (via GitHub)" <gi...@apache.org>.
Sameerali0 commented on code in PR #33965:
URL: https://github.com/apache/superset/pull/33965#discussion_r2222276465
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
The test is failing because if the user writes "bar," in the schema then there should only be a bar schema but we also have an "" empty schema with it thus the test fails. The provided solution doesnt works.
Here is the output of the failing test:
Error: The document global was defined when React was initialized, but is not defined anymore. This can happen in a test environment if a component schedules an update from an asynchronous callback, but the test has already finished running. To solve this, you can either unmount the component at the end of your test (and ensure that any asynchronous operations get canceled in componentWillUnmount), or you can change the test itself to be asynchronous.
at Object.invokeGuardedCallbackDev (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:3901:17)
at invokeGuardedCallback (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:4056:31)
at flushPassiveEffectsImpl (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:23543:11)
at unstable_runWithPriority (/app/superset-frontend/node_modules/scheduler/cjs/scheduler.development.js:468:12)
at runWithPriority$1 (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:11276:10)
at flushPassiveEffects (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:23447:14)
at Object.<anonymous>.flushWork (/app/superset-frontend/node_modules/react-dom/cjs/react-dom-test-utils.development.js:992:10)
at Immediate.<anonymous> (/app/superset-frontend/node_modules/react-dom/cjs/react-dom-test-utils.development.js:1003:11)
at processImmediate (node:internal/timers:483:21)
Node.js v20.19.4
Browserslist: browsers data (caniuse-lite) is 6 months old. Please run:
npx update-browserslist-db@latest
Why you should do it regularly: https://github.com/browserslist/update-db#readme
/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:3901
throw Error( "The document global was defined when React was initialized, but is not defined anymore. This can happen in a test environment if a component schedules an update from an asynchronous callback, but the test has already finished running. To solve this, you can either unmount the component at the end of your test (and ensure that any asynchronous operations get canceled in componentWillUnmount), or you can change the test itself to be asynchronous." );
^
Error: The document global was defined when React was initialized, but is not defined anymore. This can happen in a test environment if a component schedules an update from an asynchronous callback, but the test has already finished running. To solve this, you can either unmount the component at the end of your test (and ensure that any asynchronous operations get canceled in componentWillUnmount), or you can change the test itself to be asynchronous.
at Object.invokeGuardedCallbackDev (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:3901:17)
at invokeGuardedCallback (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:4056:31)
at flushPassiveEffectsImpl (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:23543:11)
at unstable_runWithPriority (/app/superset-frontend/node_modules/scheduler/cjs/scheduler.development.js:468:12)
at runWithPriority$1 (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:11276:10)
at flushPassiveEffects (/app/superset-frontend/node_modules/react-dom/cjs/react-dom.development.js:23447:14)
at Object.<anonymous>.flushWork (/app/superset-frontend/node_modules/react-dom/cjs/react-dom-test-utils.development.js:992:10)
at Immediate.<anonymous> (/app/superset-frontend/node_modules/react-dom/cjs/react-dom-test-utils.development.js:1003:11)
at processImmediate (node:internal/timers:483:21)
Node.js v20.19.4
Browserslist: browsers data (caniuse-lite) is 6 months old. Please run:
npx update-browserslist-db@latest
Why you should do it regularly: https://github.com/browserslist/update-db#readme
FAIL src/features/databases/DatabaseModal/index.test.tsx (260.128 s)
● dbReducer › it will set state to payload from extra
input change when schemas_allowed_for_file_upload
with blank list
expect(received).toEqual(expected) // deep equality
- Expected - 1
+ Received + 1
Object {
"backend": "postgres",
"configuration_method": "dynamic_form",
"database_name": "Postgres",
"driver": "psycopg2",
- "extra": "{\"schemas_allowed_for_file_upload\":[\"bar\"]}",
+ "extra": "{\"schemas_allowed_for_file_upload\":[\"bar\",\"\"]}",
"id": 123,
"is_managed_externally": false,
"name": "PostgresDB",
}
1755 |
1756 | // extra should be serialized
> 1757 | expect(currentState).toEqual({
| ^
1758 | ...databaseFixture,
1759 | extra: '{"schemas_allowed_for_file_upload":["bar"]}',
1760 | });
at Object.toEqual (src/features/databases/DatabaseModal/index.test.tsx:1757:26)
jest-html-reporter >> Report generated (/app/superset-frontend/test-report.html)
Summary of all failing tests
FAIL src/features/databases/DatabaseModal/index.test.tsx (260.128 s)
● dbReducer › it will set state to payload from extra
input change when schemas_allowed_for_file_upload
with blank list
expect(received).toEqual(expected) // deep equality
- Expected - 1
+ Received + 1
Object {
"backend": "postgres",
"configuration_method": "dynamic_form",
"database_name": "Postgres",
"driver": "psycopg2",
- "extra": "{\"schemas_allowed_for_file_upload\":[\"bar\"]}",
+ "extra": "{\"schemas_allowed_for_file_upload\":[\"bar\",\"\"]}",
"id": 123,
"is_managed_externally": false,
"name": "PostgresDB",
}
1755 |
1756 | // extra should be serialized
> 1757 | expect(currentState).toEqual({
| ^
1758 | ...databaseFixture,
1759 | extra: '{"schemas_allowed_for_file_upload":["bar"]}',
1760 | });
at Object.toEqual (src/features/databases/DatabaseModal/index.test.tsx:1757:26)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33965:
URL: https://github.com/apache/superset/pull/33965#discussion_r2173577303
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
### Inefficient Schema Array Filtering <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The filtering method performs unnecessary iterations and string operations by checking every element and trimming values that might be discarded.
###### Why this matters
Inefficient string operations and array traversal can impact performance when dealing with large lists of schemas. Each element undergoes a trim() operation before knowing if it will be kept.
###### Suggested change ∙ *Feature Preview*
Optimize the filtering by using a more efficient approach that only trims values when needed:
```typescript
.split(',')
.map(schema => schema.trim())
.filter((schema, index, arr) =>
index === arr.length - 1 || schema !== '')
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/026c80f6-ead1-4a20-a494-eeacebb2d859/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/026c80f6-ead1-4a20-a494-eeacebb2d859?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/026c80f6-ead1-4a20-a494-eeacebb2d859?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/026c80f6-ead1-4a20-a494-eeacebb2d859?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/026c80f6-ead1-4a20-a494-eeacebb2d859)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:ca2940e9-08ff-4119-bb6c-e3792519ea36 -->
[](ca2940e9-08ff-4119-bb6c-e3792519ea36)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "Sameerali0 (via GitHub)" <gi...@apache.org>.
Sameerali0 commented on code in PR #33965:
URL: https://github.com/apache/superset/pull/33965#discussion_r2221999651
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
The test is failing because if the user writes bar in the schema then there should only be a bar schema but we also have and "" empty schema with it thus the test fails.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz closed pull request #33965: fix(database-modal): cant enter commas into input(schemas allowed for file)
URL: https://github.com/apache/superset/pull/33965
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33965:
URL: https://github.com/apache/superset/pull/33965#discussion_r2222277608
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
The issue is that trailing commas add empty schemas. Let's modify the filter to only keep non-empty strings:
```typescript
.split(',')
.filter(schema => {
const trimmed = schema.trim();
return trimmed !== '';
})
```
This will correctly handle "bar," by keeping only ["bar"] without empty strings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(database-modal): cant enter commas into input(schemas allowed for file) [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #33965:
URL: https://github.com/apache/superset/pull/33965#discussion_r2200602087
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
Good point about empty schemas - the current implementation doesn't handle empty values correctly. We can address both performance and empty schema handling with a slight modification:
```typescript
.split(',')
.map(schema => schema.trim())
.filter(schema => schema !== '' || schema === '')
```
This will properly handle empty schemas while maintaining better performance.
##########
superset-frontend/src/features/databases/DatabaseModal/index.tsx:
##########
@@ -316,7 +316,11 @@ export function dbReducer(
...extraJson,
schemas_allowed_for_file_upload: (action.payload.value || '')
.split(',')
- .filter(schema => schema !== ''),
+ .filter((schema, index, arr) => {
+ const trimmed = schema.trim();
+ if (index === arr.length - 1) return true;
+ return trimmed !== '';
+ }),
Review Comment:
For empty schemas handling, we can modify the suggested approach to:
```typescript
.split(',')
.map(schema => schema.trim())
.filter((schema, index, arr) =>
index === arr.length - 1 || schema !== '')
|| [] // Return empty array for empty input
```
This preserves the performance benefits while properly handling empty input cases.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org