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

Confusing comment in a db migration #6200

Open
eugene-nikolaev opened this issue Jul 20, 2023 · 5 comments
Open

Confusing comment in a db migration #6200

eugene-nikolaev opened this issue Jul 20, 2023 · 5 comments

Comments

@eugene-nikolaev
Copy link

Issue Summary

Issue in the code comment (opposite meaning).

Commit title says:

Give query case sensitive treatment in query hash (https://github.com/getredash/redash/pull/4254)
Generating the query hash from the query text with no lowercasing of the query text
allows case-sensitive parameter values in the dashboard to have different cache entries.

But in the migration:

Make case insensitive hash of query text

Which have the opposite meaning.

Should be written as:

Make case sensitive hash of query text

Steps to Reproduce

Not applicable

Technical details:

No matter

@justinclift
Copy link
Member

Good point. Possibly a typo in one of the pieces. Let's ask.

@osule What are your thoughts on this? 😄

@osule
Copy link
Contributor

osule commented Jul 28, 2023

@eugene-nikolaev It should be corrected to case-sensitive hash of query text

@justinclift
Copy link
Member

Anyone want to create a PR for that? It'll be merged fairly quickly. 😄

@eugene-nikolaev
Copy link
Author

eugene-nikolaev commented Jul 28, 2023

BTW had issues which maybe caused with this migration on existing 10.1 instance.
Query results were not cached at all.
No time at the moment to dig deeper and make a proper report, I just reverted everything.

Also reverting from current master to 10.1 back required some hacking as some strings did not fit in that limit=320.

op.add_column("users", sa.Column("profile_image_url", db.String(320), nullable=True))

Hopefully I'll be able to tinker with it and create proper issues/PRs.

@justinclift
Copy link
Member

Ouch. Yeah, our current CI tests are good for some stuff. But we really need to also set up some more in-depth automatic testing with actual proper test data and have it run "like" it would in production.

I have some ideas, but it's not something I'll get to this week personally. Fixing more immediate stuff first. 😄


Btw, when you do do want to tinker with stuff then we've recently been working on decent instructions for setting up your local dev environment:

https://github.com/getredash/redash/wiki/Local-development-setup

The stuff on the wiki is known to work, whereas the "developer setup guides" (currently) on the official Redash knowledge base are still pretty busted. We'll need to migrate the wiki content there at some point, but it's not a high priority (yet). 😄

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

No branches or pull requests

3 participants