madhushreeag commented on PR #40178: URL: https://github.com/apache/superset/pull/40178#issuecomment-4484505927
> @madhushreeag some questions/comments before I post a full review: > > * do we expect there to be duplicates in the user list? I'm surprised this would be the case, as I assume we have unique indexes in place on the relevant metastore tables. > * If the frontend now deduplicates users, do we need the backend change? Rather than override the FAB method, I'd prefer to upstream any critical fixes to FAB so FAB gets the fixes + we don't have override logic in place that may become stale as FAB changes its internals > * I'm surprised that the PUT would overflow with a few thousand user ids. If this is the case, we may need to change our request size limits to accommodate for larger roles. You're right that the DB has unique indexes — there are no duplicate rows in ab_user_role. The duplicates are a pagination artifact, not a data integrity issue. Since fetchPaginatedData fires all page requests concurrently via Promise.all, the same row can fall on page 3 in one request and page 4 in another, depending on what the query planner does at that instant. The fix is ORDER BY id ASC on the users fetch, which makes page boundaries stable. The deduplication is just a safety net. I can still remove the dedupe if it feels redundant. The backend changes have been removed from this PR. The ORDER BY fix eliminates duplicates at the source, so there's nothing incorrect being sent to FAB — no backend override needed. Agreed that a few thousand integer IDs shouldn't overflow a default request size limit — a role with 950 users is only a few KB in the request body. We haven't hit 413 in practice with the current fix in place. We were seeing 404 due to the pagination 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
