Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Bump YARL dependency to retain trailing slashes in URLs #267

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

oliverlambson
Copy link

@oliverlambson oliverlambson commented Mar 14, 2024

As per #244, the superset-cli command appears to be incompatible with Superset >=3.0.0

In running superset-cli with loglevel=debug, this seems to be to due redirects causing some weirdness with the Auth header. See my post on that issue for the logs: #244 (comment).

The redirects occur because yarl <1.9.4 drops trailing slashes (see aio-libs/yarl#862 and aio-libs/yarl#866), which means the generated request URLs do not exactly match the Superset API docs

@oliverlambson
Copy link
Author

oliverlambson commented Mar 14, 2024

@betodealmeida would appreciate a review on this, many thanks

@Vitor-Avila
Copy link
Contributor

Hey @oliverlambson, thanks for submitting these changes! I'm afraid that due to license restrictions, we're currently not able to accept/merge contributions from outside of Preset. I'll take a look on this scenario and see if we can address this issue. Thanks!

@oliverlambson
Copy link
Author

Hey @oliverlambson, thanks for submitting these changes! I'm afraid that due to license restrictions, we're currently not able to accept/merge contributions from outside of Preset. I'll take a look on this scenario and see if we can address this issue. Thanks!

Thanks!

@CLAassistant
Copy link

CLAassistant commented Mar 19, 2024

CLA assistant check
All committers have signed the CLA.

@Vitor-Avila
Copy link
Contributor

Hey @oliverlambson,

Our team has been working to implement this CLA that allows us to accept external contributions to the CLI -- thank you for singing it 🙌

I was performing some tests around this flow, and I couldn't reproduce the issue in the first place. I'm not opinionated against the proposed fix, but I was wondering if I could reproduce to check if we could add test coverage to prevent any regression in the future. This is how I tried to reproduce it:

  1. Created a new Python virtual environment using Python v3.9.0.
  2. Installed Superset v3.0.0 through pip.
  3. Tried using it with the CLI right away (command below). Faced a continuous loop that never really threw any errors (in the CLI). In the Superset logs, I could find flask_wtf.csrf.CSRFError: 400 Bad Request: The CSRF session token is missing..
superset-cli --loglevel=DEBUG -u admin -p admin http://localhost:8088 export-assets --chart-ids=101 . --overwrite
  1. I thought this could be the issue, but then I ran pip install --upgrade yarl==1.9.4 in the CLI env and I was still facing the same issue.
  2. Downgraded yarl to the previous version, and then updated below configs in Superset:
WTF_CSRF_ENABLED = False
SESSION_COOKIE_SAMESITE = None
  1. Sync command still failing. Upgraded yarl again, still same issue.
  2. Lastly, set TALISMAN_ENABLED = False in the Superset config, and then the sync was back working (with the old yarl version).

I also tried using ngrok to force an https URL but also couldn't reproduce the issue.

Do you have any specific steps I could follow to repro it? Do you know if this could be related with your CSP/Talisman config?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants