Incubator-superset: Owner names not rendered in React Datasource editor

[john-bodley]

Expected results

The dataset owners should be rendered.

Actual results

How to reproduce the bug

1. Go to a chart.
2. Click on 'Edit Datasource'
3. Click on the 'SETTINGS' tab
4. Scroll to the bottom and and noticed that the owner names aren't rendered

Environment

(please complete the following information):

• superset version: master
• python version: 3.7
• node.js version: 12

Checklist

Make sure to follow these steps before submitting your issue - thank you!

• [x] I have checked the superset logs for python stacktraces and included it here as text if there are any.
• [x] I have reproduced the issue with at least the latest released version of superset.
• [x] I have checked the issue tracker for the same issue and I haven't found one similar.

[john-bodley]

@rusackas I believe this regression may be related to a recent change from Preset related to the Antd refactor.

[rusackas]

@kgabryje

[rusackas]

Thanks John, will investigate.

[junlincc]

@john-bodley hey John, im not able to reproduce it, do you have an idea which PR might cause it and when did you first notice it?

[john-bodley]

@junlincc unsure. We believe this regression may have been introduced in one of these changes.

[rusackas]

@junlincc unsure. We believe this regression may have been introduced in one of these changes.

I'm not able to repro this either yet. I would try a little git bisect action, but I need a failing state to do so. :/

[junlincc]

@john-bodley could you help narrow it down a bit 🙏

@junlincc unsure. We believe this regression may have been introduced in one of these changes.

I'm not able to repro this either yet. I would try a little git bisect action, but I need a failing state to do so. :/

[john-bodley]

@junlincc I'm going to see if the issue is present in our internal weekly deploy which occurs on Wednesday. If it's resolved it's likely our last week branch cut did not contain the commit and I'll close the issue, otherwise I'll dig into this in more detail.

[john-bodley]

@junlincc and @rusackas I'm able to reproduce this from master using our production data. It seems like the issue is the fetched set of potential owners is quite restricted (the CRUD view fetches all users) and the prescribed owners are not within said set and thus the labels aren't rendered. My hunch is you may not be able to repo it as the corpus of users is defined entirely within the null state response.

The logic is defined here and I only see one request to the api/v1/dataset/related/owners endpoint regardless of the typeahead state.

[junlincc]

@john-bodley thanks for the additional info we are looking into it!

[graceguo-supercat]

I found another JS bug for owners selector (besides the issue John just reported). this can be easily reproduced in open source master branch:

1. if the owners list is empty, add one or more names in the owners list.
2. then remove all owners one by one. on removal the last one from owners list, you will see js error.

[kgabryje]

@junlincc Can you assign me please?

[kgabryje]

@john-bodley I think that this issue might have been introduced by https://github.com/apache/incubator-superset/pull/11221. I'm not able to verify it as I can't directly reproduce this bug with my test data. Is it possible that the new endpoint api/v1/dataset/related/owners returns fewer owners than the old /users/api/read and your data actually uses the missing users? Could you please verify it?
CC @lilykuang @junlincc

[john-bodley]

@dpgaspar do you know of any differences between the /api/v1/dataset/related/owners and /users/api/read API endpoints?

[nytai]

One difference is that /api/v1/dataset/related/owners is paginated (with pageSize and pageIndex params) while I'm guessing /users/api/read is not paginated. My guess is that airbnb is hitting this issue because they have >25 users which is the default page size. The /api/v1/dataset/related/owners also supports searching via filter param.

My guess is that because the existing owners is not in the list of select options it's resulting in this weird rendering issue. We may want to consider just switching back to /users/api/read or trying to use the pagianted select component which may fix this issue

[kgabryje]

@nytai I think your guess is correct - I could reproduce John's bug by limiting a page size.
I wonder if paginated endpoint has any value for us in this case? We always need to load all options to make sure that we can display all selected values. Unless we implemented some logic "keep loading next pages until all saved values have a matching option" - though it doesn't sound like this optimization is worth the effort.

[etr2460]

@nytai @kgabryje @junlincc:

Are there any updates on resolving this? It seems like Tai had a few options for fixing the problem, is anything else blocking trying to fix it?

[nytai]

@kgabryje That's a great question. I have spend quite a bit of time working around this (adding PaginatedSelect) and have often questioned the value of paginating these payloads. @dpgaspar do you have any thoughts around removing pagination for these endpoints?

[nytai]

We could also send the request with page_size=2000 or something large enough to solve this issue for most practical cases.

[dpgaspar]

We will probably go with page_size=2000, and address this properly next. A possible solution is to create a small feature on the generic related endpoint to support something like:

{"filter": "<text to filter>", "ids": [1,2,4,n], page:X, page_size:Y}

Where the endpoint would get filtered paginated data and make sure the requested ids were included on the response also. At a probable cost of a further query but still more efficient then sending back a very large payload