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!

• I have checked the superset logs for python stacktraces and included it here as text if there are any.
• I have reproduced the issue with at least the latest released version of superset.
• 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 #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

[zuzana-vej]

FYI, the above linked PR didn't resolve the issue.

See owners on "Edit Dataset" modal in Explore:

See owners on the Data --> Dataset page for same dataset

[codenamelxl]

Changing this into api/v1/dataset/related/owners?q=(page_size:2000) will be a quick fix.

superset/superset-frontend/src/datasource/DatasourceEditor.jsx

Line 629 in b489cff

dataEndpoint="api/v1/dataset/related/owners"

Side issue: No request to /dataset/related/owners was made when typing into Owner field as well.
Behavior for Chart and Dashboard is it trigger /related/owners on every keytype.
This is due to:

superset/superset-frontend/src/components/AsyncSelect/index.jsx

Line 88 in b489cff

<Select

using Select instead of AsyncSelect.

[etr2460]

Hey @amitmiran137 , is Neilsen still planning on working on this? Thanks!

[etr2460]

@michael-s-molina do you think this is something that your async select work could help with? This seems to be a regression dating back to the original Antd refactor

[michael-s-molina]

@michael-s-molina do you think this is something that your async select work could help with? This seems to be a regression dating back to the original Antd refactor

@etr2460 Yes. I'm tagging @geido here because he worked on this migration and has the full context.

[geido]

@etr2460 I don't think my current implementation would work for the Datasource settings specifically. You can check the code in this PR #16609 that is related to the owners in the Datasource settings. Basically, we would need a refactor of the SelectAsyncControl to work with the pagination. That PR is not merged yet so this might be a good opportunity to think about doing it, depending on prioritization OR we can increase the pagesize to a high limit as a quick fix as suggested above. CC @junlincc

There is another implementation which is using the paginated fetch instead and that's in the Properties modal. I have an env with more than 10 owners (10 as a pagesize limit) and could not spot any problem. I am about to merge this PR containing the changes. This can be a good opportunity to validate this issue against the new implementation #16445.

[nytai]

there is a workaround to use the Legacy Datasource Editor. Aside from the list of owners not being rendered properly, this bug does block adding new owners beyond the 25 that are initially fetched which is a major issue. This should be fixed before we fully deprecate the legacy FAB views

[michael-s-molina]

@junlincc @nytai Can you add some context about the reopening of this issue? I just checked it on master and it seems to be working as intended. I simulated a page with 10 results.

Screen.Recording.2021-12-01.at.9.56.28.AM.mov

[nytai]

This is still an issue. @michael-s-molina please try with > 25 results

[nytai]

The call to fetch owners is /api/v1/dataset/related/owners which is falling back to the FAB default limit of 20 (https://github.com/dpgaspar/Flask-AppBuilder/blob/314c3482b067f2fd3a2b29c222a2c1a8410d61e6/flask_appbuilder/api/__init__.py#L959)

In order to repro this bug, you will need:

1. To have more than 20 possible owner values
2. Set an owner.
3. Make sure the set owner is not part of the initial call that fetches 20 results (might be easier to just set all users as owners).
4. You will see the empty select option

[nytai]

this appears to be fixed in the latest version (1.4.0)?

[nytai]

Yup, looks like it was fixed by #17063