fix(databases): GSheets and Clickhouse DBs are not allowed to upload files #21065
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21065 +/- ##
==========================================
- Coverage 66.61% 66.61% -0.01%
==========================================
Files 1791 1791
Lines 68559 68586 +27
Branches 7319 7325 +6
==========================================
+ Hits 45671 45686 +15
- Misses 21002 21017 +15
+ Partials 1886 1883 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Antonio-RiveroMartnez
force-pushed
the
FIX-39367
branch
2 times, most recently
from
42a1a0a
to
104b76c
Compare
superset/views/database/forms.py
Outdated
|
|
||
| class DatabasesNotAllowedToFileUpload(str, Enum): | ||
| GSHEETS = "gsheets" | ||
| CLICKHOUSE = "clickhousedb" |
There was a problem hiding this comment.
Rather than maintaining a separate enum for this, why not define this as a property on the db_engine_spec?
There was a problem hiding this comment.
Hey @nytai! I just updated the PR with such property in our base and each engine now. Thanks for the suggestion.
There was a problem hiding this comment.
@nytai let us know if you want us to recruit additional folks for a review, or if you have time to take a look. Thanks!
There was a problem hiding this comment.
I think having someone with more experience with these per-db configs, like @villebro or @john-bodley, have a look would be good.
There was a problem hiding this comment.
@Antonio-RiveroMartnez Thanks for making that change, multiple places in the frontend where you're checking
db?.engine !== Engines.GSheet && db?.engine !== Engines.ClickHouse. This makes adding a new engine that does not support file upload a pain, since you have to update a bunch of files across both backend and frontend code. Ideally one could just specify in the db_engine_spec allows_file_upload=False and that's it. So maybe adding a new api that you could call from the db modal to check is a supplied connection string or config supports file uploads would be a better approach?
There was a problem hiding this comment.
I agree with @nytai here - we should place this flag in the db engine specs, and the UI should accommodate accordingly. I may be wrong, but I believe we're already sending some db engine spec metadata in bootstrap data. Maybe we could just add allows_file_upload to BaseEngineSpec, override it in the GSheets and ClickHouse specs, and then make sure the flag is made available in bootstrap data? If we're not passing any info about db engine specs, now would probably be a good time to do so.
There was a problem hiding this comment.
Hey @nytai @villebro! Yeah, currently the flag is being set in our BaseEngineSpec and being overridden in our GSheets and Clickhouse specs. The missing part is sending it (or using it) in the frontend instead of db?.engine !== Engines.GSheet && db?.engine !== Engines.ClickHouse. So, let me check the bootstrap data and if not doing it already let me start sending it. Thank you both for the input.
There was a problem hiding this comment.
Ok @nytai @villebro , I updated the PR, we were sending some extra information in the response but that might is editable by the user so I decided to include a new engine_information that currently only has allows_file_upload flag. Now, There is no need for updating multiple places but instead just setting such flags as true to prevent file uploading.
Antonio-RiveroMartnez
force-pushed
the
FIX-39367
branch
2 times, most recently
from
9557cd8
to
d5c12ad
Compare
|
/testenv up |
|
@yousoph Ephemeral environment spinning up at http://54.201.86.244:8080. Credentials are |
Antonio-RiveroMartnez
force-pushed
the
FIX-39367
branch
3 times, most recently
from
edeb16c
to
a9bb5c2
Compare
|
/testenv up |
|
@yousoph Ephemeral environment spinning up at http://54.213.205.154:8080. Credentials are |
superset/db_engine_specs/base.py
Outdated
| :returns: Dict with properties of our class like allows_file_upload | ||
| """ | ||
| return { | ||
| "allows_file_upload": cls.allows_file_upload, |
There was a problem hiding this comment.
how is this different from the current colum allow_file_upload? If they are very different, then should we think about changing the name of this?
allows_file_upload and allow_file_upload being different columns that serve different purposes is going to be way too confusing.
There was a problem hiding this comment.
The allow_file_upload is in the Database model and it's set per each user's DB. While the allows_file_upload is set in the DB Engine Spec model and cannot be set by the user. So, even when a Database has its flag set to true (think of existing user's DB), if its DB Engine Spec doesn't support it, we prevent file uploading to it.
There was a problem hiding this comment.
ok that makes sense!
Could we name it supports_file_upload or something along the like? Having two columns only separated by a singular/plural shift is going to be confusing.
There was a problem hiding this comment.
Yeah, totally! Let me make the change and ILYK. Thanks for the input @AAfghahi
There was a problem hiding this comment.
of course! Otherwise this LGTM!
There was a problem hiding this comment.
PR Updated with the new supports_file_upload @AAfghahi :)
Antonio-RiveroMartnez
force-pushed
the
FIX-39367
branch
2 times, most recently
from
ce106d4
to
0fafb1e
Compare
|
Storybook has completed and can be viewed at |
- GSheets and Clickhouse databases are not allowed to upload files to it so we need to stop showing such options to the users. This needs to happens for new DBs and existing ones. Also include Clickhouse DB as not allowed for data upload.
- Add a new property in db_engine_spec so we can tell whether a db engine support file upload. It is True in by default. - Stop using an Enum and instead make the method that populates the dropdown in our form use the new property for the engine of a given database
- The text input Schemas allowed for File upload renders conditionally based on whther the user selects Allow file upload. - Fix margins for checboxes and new location of the text input - Update the wording for Allow file upload and text input for schemas - Remove tooltip from Allow file upload checkbox - Update tests
- Include a new engine_information property in the response got our GET apis (single and list). Currently inside that property we send the allows_file_upload config value for the specific engine but it can added more information later.
- Instead of manually checking if engine is GSheets or Clickhouse, use the new property so when new engines are added we dont need to change multiple file but just the property for it.
- Update our tests and openapi spec to consider the new property
- Rename allows_file_upload to supports_file_upload so we avoid confusion from the DB property
Antonio-RiveroMartnez
force-pushed
the
FIX-39367
branch
from
0fafb1e
to
8ae1d39
Compare
- Update our new RightMenu tests to include the environmentTag required prop
|
Ephemeral environment shutdown and build artifacts deleted. |
| @@ -118,8 +118,8 @@ const RightMenu = ({ | |||
| ALLOWED_EXTENSIONS, | |||
| HAS_GSHEETS_INSTALLED, | |||
| } = useSelector<any, ExtentionConfigs>(state => state.common.conf); | |||
| const [showModal, setShowModal] = useState<boolean>(false); | |||
| const [engine, setEngine] = useState<string>(''); | |||
| const [showModal, setShowModal] = React.useState<boolean>(false); | |||
There was a problem hiding this comment.
What is the reason for using this versus importing in useState and invoking it normally?
There was a problem hiding this comment.
So we can spyOn during our tests, e.g. const useStateMock = jest.spyOn(React, 'useState'); https://github.com/apache/superset/pull/21065/files#diff-6a8138bf7b3a4c95d31569dfe4497ceb65ae07faddc47fb944a611ec7fb19776R86
There was a problem hiding this comment.
If you look at the line above that for useSelector, could you follow that method instead and import useState?
There was a problem hiding this comment.
You cannot because if you do something like:
jest.mock('react', () => ({
...jest.requireActual('react'),
useState: jest.fn(),
}));
the whole tests are going to fail because React itself wont have all what it needs to render and handle cases where we actually need regular useState, and after a while researching I found that spying on native hooks from react would require such usage as we have here. I guess it's the same reason why this other component uses it already in our code base:
There was a problem hiding this comment.
But that test and file still imports useState and uses it instead of doing React.useState
There was a problem hiding this comment.
Indeed but the usage of the spied hook is different which might be affecting why these tests cannot do it that way, either way I'll give a spin to see if it works that way, if not possible we need to decide whether we move forward with it as is or not 🤔 I'll keep you posted @AAfghahi , thanks for the input 🎉
mistercrunch
added
🏷️ bot
SUMMARY
Our users are getting an
Internal Server Error 500when trying to upload files toGSheetsdatabases, that's because such databases don't support file upload nor theClickhouseones thus we need to stop showing such options to the users and prevent that existing ones get displayed in the upload form.Additionally,
schemas allowedfield is dependent on having file upload enabled but doesn't appear that way, so we are rendering it only when user checks theAllow file uploadcheckbox and placing it underneath the file upload checkbox.Wording changes:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
Allow data uploadcheckbox is not displayed in the formGSheetsnorClickhouseDatabasedropdown must not contain anyGSheetsnorClickhouseDB.ADDITIONAL INFORMATION