feat(sqllab): SPA migration

[justinpark]

SUMMARY

Part 4-9 of SQLLab SPA migration work: detail link

• Change the url from /superset/sqllab to /sqllab
• Update frontend routes
• Update unit tests and cypress e2e tests
• Update redirectSQLLab logic

For the redirect logic, it will replace the SupersetClient.post redirection by react-router redirection with the location state for the post payload data. This means the redundant form_data serialize/deserialize logic has been discarded (which is also another performance gain).

FYI: This commit does not include the delete work (i.e. old entrypoint related files, redirect legacy endpoint to new endpoint). This work will be handled in the next PR to minimize the regression rollback.

TESTING INSTRUCTIONS

1. Make sure the SQL Lab page works as before but in the context of SPA (does not reload the page when navigating)
2. Make sure that /superset/sqllab redirects to /sqllab including additional query parameters

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[eschutho]

FYI: This commit does not include the delete work (i.e. old entrypoint related files, redirect legacy endpoint to new endpoint). This work will be handled in the next PR to minimize the regression rollback.

@justinpark are you removing any old api endpoints in the next PR? If so we may want to wait until 4.0. I think there may be some integrations outside of the application that use it, so it could be a breaking change. cc @betodealmeida for the CLI that I think uses this.

[justinpark]

FYI: This commit does not include the delete work (i.e. old entrypoint related files, redirect legacy endpoint to new endpoint). This work will be handled in the next PR to minimize the regression rollback. @justinpark are you removing any old api endpoints in the next PR?

After discussed with other folk, I decided to include the delete work in the same PR. I will post the commit for the delete work soon.

If so we may want to wait until 4.0. I think there may be some integrations outside of the application that use it, so it could be a breaking change. cc @betodealmeida for the CLI that I think uses this.

This is a web entry point migration not API so it shouldn't be used in CLI. (even it's used in CLI, the redirection will cover the issue)

[michael-s-molina]

Hi @eschutho. Just to add a little more context. I asked @justinpark to include the cleanup of old code in the same PR given that:

• Some features might work because of old code and they shouldn't. We should validate SPA considering that the old code has no influence in the project.
• The PR is big and requires a lot of testing and validation. It's good if we do this only once. If we do the cleanup work later, we'll need to repeat the same extensive test process.

Just to be clear, removing old code shouldn't introduce breaking changes. Let's make sure this is the case when reviewing the PR.

Once code review is completed, we'll create a test environment to make sure everything is working. It would be great if @sadpandajoe and @jinghua-qa could help us out.

[michael-s-molina]

Thank you for the PR @justinpark. I left some first-pass comments.

[justinpark]

@michael-s-molina I just changed all addressed issues.

[kgabryje]

Code looks good! Great to see the old obsolete code gone

[kgabryje]

/testenv up

[github-actions]

@kgabryje Ephemeral environment spinning up at http://34.219.198.150:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

[eschutho]

@justinpark and @michael-s-molina This all looks good to me. I didn't see any breaking changes in the PR, as the current endpoints have all the redirects in place as @justinpark mentioned. It's only the deleting of the endpoints that would raise any concerns for breaking changes, which I didn't see in here. 👍

[michael-s-molina]

Thank you @eschutho and @kgabryje. Code LGTM too.

@sadpandajoe @jinghua-qa Could you also review the PR before we merge it?

[justinpark]

@sadpandajoe @jinghua-qa Could you help QA test for SPA migration work?

[sadpandajoe]

@sadpandajoe @jinghua-qa Could you help QA test for SPA migration work?

Sure @justinpark. Bring up an ephemeral. Also what have you tested so far so we don't duplicate testing.

[justinpark]

Sure justinpark. Bring up an ephemeral. Also what have you tested so far so we don't duplicate testing.

@sadpandajoe Ephemeral environment already set up at http://34.219.198.150:8080/.

We completed the basic page transition tests(from chart to sqllab. etc) so I hope you test running queries and make a bunch of page transition and page reload that make sure no data lost or error thrown.

[justinpark]

@sadpandajoe Do you have any updates in your side?
We're planning to merge it this Wednesday if you don't have any inputs by then.

[sadpandajoe]

@sadpandajoe Do you have any updates in your side? We're planning to merge it this Wednesday if you don't have any inputs by then.

@justinpark I haven't had a chance to really look at it yet since I got stuck doing other things. I think things I was going to look at for testing include:

• Saved queries links still work
• The different navigations back to sql lab like on a chart where you can view query in sql lab
• Stopping queries that are running

But probably won't have time to get to it until the later half of this week as I need to finish some other stuff. I would say merge and if I find anything I'll just create new issues and ping you on them.

[eschutho]

I did a quick test on the ephemeral, and it's very snappy! 🙌
I also noted what looks like new functionality when you select a table from autocomplete in the sql ace editor that the table is automatically selected in the left column. Very nice!

[justinpark]

But probably won't have time to get to it until the later half of this week as I need to finish some other stuff. I would say merge and if I find anything I'll just create new issues and ping you on them.

Sounds good! We've completed reasonable tests so I'll take cake in the following bug fixes for your reported bugs.

[geido]

@justinpark code LGTM.

Not sure if all of these are related, actually this might all be unrelated, but for reference here is what I found from manual testing and messing up with SQL Lab

1. The duplicate tab does not recognize tab name and goes with "Untitled". Also, when going back to previous tab the result set is emptied

Superset.mp4

2. Wnet from saved queries to SQL Lab but the menu is still highlighting Saved Queries. Same happens with query history

3. Again, this is probably unrelated but when I have multiple tabs, if I select "Query History" in one tab it will move all the other tabs to "Query History". I think this is related to the video above. For instance, if I am viewing the definition of a table and then switch to a different query it will go empty and it will make the table definition disappear in the originating tab

Superset.1.mp4

4. I found queries in Query history with no title, which will correspond to a tab named "Copy of" in SQL Lab.

5. Realized that you can name a query with an empty space (100% unrelated)

6. I saw that we lost the ability to forcefully open the query in a new browser tab from Explore -> Run in SQL Lab. I think the user should still be able to choose whether to open it in a new browser tab or not. Same happens in Saved Queries but not in Query History

[michael-s-molina]

Thank you for all the tests and comments @geido!

@justinpark Given that this PR contains 54 changed files, I highly recommend we fix the bugs that are related to this PR, merge the PR, and then open follow-ups for bugs that already exist on master. The reason for this suggestion is to facilitate the review process given the size of the PR.

[justinpark]

Not sure if all of these are related, actually this might all be unrelated, but for reference here is what I found from manual testing and messing up with SQL Lab

1. The duplicate tab does not recognize tab name and goes with "Untitled". Also, when going back to previous tab the result set is emptied

This is an existing bug and will follow up in the separate PR.

Superset.mp4
2. Wnet from saved queries to SQL Lab but the menu is still highlighting Saved Queries. Same happens with query history

I think you navigate back/forth by the browser navigation (not menu). This is same issue on existing tabs (went from Saved Queries to Query History) and will follow up in the separate PR.

3. Again, this is probably unrelated but when I have multiple tabs, if I select "Query History" in one tab it will move all the other tabs to "Query History". I think this is related to the video above. For instance, if I am viewing the definition of a table and then switch to a different query it will go empty and it will make the table definition disappear in the originating tab

This is how SQL Lab designed before (because activeSouthPaneTab not belong to each tab but belong to the entire sqllab). Should follow up in the separate PR.

4. I found queries in Query history with no title, which will correspond to a tab named "Copy of" in SQL Lab.

Same as no. 1

5. Realized that you can name a query with an empty space (100% unrelated)

This is an existing bug and will follow up in the separate PR.

6. I saw that we lost the ability to forcefully open the query in a new browser tab from Explore -> Run in SQL Lab. I think the user should still be able to choose whether to open it in a new browser tab or not. Same happens in Saved Queries but not in Query History

The basically it should support cmd + click that can open the link in a new browser tab but some links(which you mentioned) require a post data to retrieve the sql context so hard to maintain in SPA approach. I hope this is not a blocker of running SPA and want to discuss in the next thread(PR) for the support of opening new tab.

[justinpark]

@rusackas @eschutho @geido could you help to stamp the PR to unblock the reviewer req?

[rusackas]

Approving cypress changes as codeowner

[rusackas]

Oh, and THANK YOU for making this huge stride in the SPA effort. This is a big deal, and we're almost at the end of the tunnel! :D

[github-actions]

Ephemeral environment shutdown and build artifacts deleted.