-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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.
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
refactor: external metadata fetch API #16193
refactor: external metadata fetch API #16193
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16193 +/- ##
==========================================
- Coverage 76.75% 76.71% -0.05%
==========================================
Files 996 997 +1
Lines 53054 53073 +19
Branches 6760 6740 -20
==========================================
- Hits 40721 40714 -7
- Misses 12107 12130 +23
- Partials 226 229 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - one minor question before pressing approve
Object.keys(params).map(key => | ||
params[key] === undefined ? null : params[key], | ||
), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to encode the key/value pairs (Object.entries
), not just the values (Object.keys
)? (honest question, I haven't done a lot of this rison API stuff)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is to code entire params
field, just change undefined
to null
, because prison can't encode undefined value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes now I see! 🤦
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, and special thanks for the comprehensive tests!
…gies * upstream/master: (64 commits) check roles before fetching reports (#16260) chore: upgrade mypy and add type guards (#16227) fix: pivot columns with ints for name (#16259) chore(pylint): Bump Pylint to 2.9.6 (#16146) fix examples tab for dashboard (#16253) chore: bump superset-ui packages to 0.17.84 (#16251) chore: Shows the dataset description in the gallery dropdown (#16200) fix(Dashboard): Omnibar dropdown visibility and keyboard commands (#16168) chore: bump py version for integration test (#16213) fix: skip perms on query context update (#16250) refactor: external metadata fetch API (#16193) feat(dao): admin can remove self from object owners (#15149) fix(dashboard): cross filter chart highlight when filters badge icon clicked (#16233) fix: validate_parameters and query (#16241) fix: Remove Advanced Analytics tag for 2 charts (#16240) Revert "feat: Changing Dataset names (#16199)" (#16235) feat: Allow users to connect via legacy SQLA form (#16201) fix: remove encryption from db params (#16214) fix(Explore): Show the tooltip only when label does not fit the container in METRICS/FILTERS/GROUP BY/SORT BY of the DATA panel (#16060) Show/hide tooltips (#16192) ... # Conflicts: # superset/tasks/caching/cache_strategy.py
from typing_extensions import TypedDict | ||
|
||
|
||
class ExternalMetadataParams(TypedDict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@villebro @zhaoyongjie I wonder if we should be using marshmallow_dataclass instead as it adhere's to the DRY principle and simplifies the code. I've used it for an internal project and I'm definitely a fan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the mention. It's a more elegant way, let me try to use marshmallow_dataclass
to refactor this class.
* refactor: external metadata api * fix comments * fix ut * fix fe lint * fix UT * fix UT (cherry picked from commit 6cd15d5)
* refactor: external metadata api * fix comments * fix ut * fix fe lint * fix UT * fix UT
* refactor: external metadata api * fix comments * fix ut * fix fe lint * fix UT * fix UT
SUMMARY
closes: #16172
When special characters("/") are encountered, the API
/datasource/external_metadata_by_name/<datasouce_type>/<database_name>/<schema_name>/<table_name>
cannot be called.This PR refactor this API to
/datasource/external_metadata_by_name/?q=<rison string>
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After
virtual_dataset_sync.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION