-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix(tags): tags api change tag_get_objects method to be aligned with api documentation #29338
base: master
Are you sure you want to change the base?
Conversation
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29338 +/- ##
===========================================
+ Coverage 60.48% 83.75% +23.26%
===========================================
Files 1931 518 -1413
Lines 76236 37624 -38612
Branches 8568 0 -8568
===========================================
- Hits 46114 31512 -14602
+ Misses 28017 6112 -21905
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The existing endpoint should work, you just need to pass it a single Tag ID in the list, rather than a longer list of tag IDs. This would be a breaking change, as you're removing support for supplying the API with multiple Tag IDs. The real change we need to make is to the documentation. The docs say the API expects |
@rusackas, thanks for your feedback. I think somehow you have a point here. I thought it was the correct way to change the behavior as the OpenAPI specification is a public interface and expects a totally different variable type here: int vs array, different name, and different type: path vs parameter. But for me, it is also fine to consider that the bug is that the API is not properly documented and to fix this. I will hand something in for it. |
…umentation" This reverts commit 2804ee0.
…tual api implementation
SUMMARY
I just came across an issue where the implemented logic was not aligned with the API documentation on OpenAPI. As a result, the method was not usable. I modified the method to align with the documentation, although I saw a benefit in the initial implementation of a better filter mechanism and bulk requests. I think a breaking change should be discussed first; therefore, I removed the non-functional code here, so that we have a fixed api soon.
TESTING INSTRUCTIONS
Start up superset and use the
tags/get_objects/<tag_id>
method it now should return the charts that belong to a tag.ADDITIONAL INFORMATION