You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/04/28 05:17:00 UTC
[GitHub] [incubator-superset] bkyryliuk opened a new pull request #9667: Fix api limits in react CRUD
bkyryliuk opened a new pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667
### CATEGORY
Choose one
- [X] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
Currently react CRUD is only limited to the 20 items for the api requests.
This PR sets page size to -1 to remove this restriction.
This bug makes filtering by related fields unusable.
### TEST PLAN
Tested locally
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@dpgaspar
@nytai
@villebro
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] bkyryliuk commented on pull request #9667: Fix api limits in react CRUD
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667#issuecomment-620918408
@nytai - yes that's a good summary.
I also have a slight concern about the current API defaults, but that's out of scope of this issue.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] villebro commented on pull request #9667: Fix api limits in react CRUD
Posted by GitBox <gi...@apache.org>.
villebro commented on pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667#issuecomment-620777311
@nytai do you have any thoughts on this?
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] dpgaspar commented on pull request #9667: Fix api limits in react CRUD
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667#issuecomment-620744139
I don't like it, the API should not return all records (except for really exceptional cases) by default.
The related endpoint also supports pagination, can you add a bit more context here.
@suddjian maybe you should look at this
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] suddjian commented on pull request #9667: Fix api limits in react CRUD
Posted by GitBox <gi...@apache.org>.
suddjian commented on pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667#issuecomment-620840959
The react views are currently in a sort of "beta state" behind a feature flag, so I think changing the default limit on all endpoints as a hotfix for them is inappropriate. We should instead fix the react views to paginate correctly.
If making the endpoint unlimited really is necessary as a short-term fix, it would be better to at least limit this to the specific endpoint(s) that are causing issues.
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] bkyryliuk commented on pull request #9667: Fix api limits in react CRUD
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667#issuecomment-620750783
@dpgaspar I agree that this is imperfect and meant to be only a stop gap solution. Ideally CRUD react needs to know how use pages. This change still allows to specify the page in the url params. Current implementation truncates the results and seems to me as a dangerous behavior as leads to the silent failures that are hard to catch in unit tests & on the dev setup.
> I don't like it, the API should not return all records (except for really exceptional cases) by default.
> The related endpoint also supports pagination, can you add a bit more context here.
>
> @suddjian maybe you should look at this
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] bkyryliuk commented on pull request #9667: Fix api limits in react CRUD
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667#issuecomment-620847600
that's a bummer, will wait for a proper fix. thanks for the feedback.
To bring some clarification: this change is not disabling the pagination but rather changes the default behavior from returning the truncated data to a full data. From my perspective having functional but slow endpoint is more important than fast one but with incorrect / incomplete results. And as I mentioned current set of defaults make catching those bugs really hard in tests and dev setup as it requires api endpoint to return over 20 items
----------------------------------------------------------------
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.
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
[GitHub] [incubator-superset] nytai commented on pull request #9667: Fix api limits in react CRUD
Posted by GitBox <gi...@apache.org>.
nytai commented on pull request #9667:
URL: https://github.com/apache/incubator-superset/pull/9667#issuecomment-620884082
Just to reiterate, so I'm not missing anything. The issue here is related to the user list dropdowns that's basically the owners list right now, correct? So a solution would be to add infinite scroll and filter results asynchronously when searching? This sounds reasonable and something we'd definitely want to do. Thanks for flagging this @bkyryliuk, we haven't hit the scale at which this is a problem for us yet so it's really useful to have the issues surfaced.
If the above solution sounds reasonable I'll file an issue in the repo.
----------------------------------------------------------------
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.
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