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

Give query case sensitive treatment in query hash #4254

Merged

Conversation

osule
Copy link
Contributor

@osule osule commented Oct 16, 2019

What type of PR is this?

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

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.

Related Tickets & Documents

Fixes #2137

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening this pull request. I've added some comments in case you are interested to follow up on it.

@osule osule force-pushed the 2137-case-insensitive-result-cache branch from a691447 to 884e354 Compare March 1, 2020 17:01
@osule osule requested a review from arikfr March 1, 2020 17:04
@guidopetri
Copy link
Contributor

@osule , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

@justinclift
Copy link
Member

justinclift commented Jul 16, 2023

Looking over this PR, the requested changes were made so it should be ok to approve.

Looks like it's not passing our CI tests though, so some tweaking seems like it's still needed.


The failing CI seems to be a GitHub Actions infrastructure problem (yet again):

URL: https://download.cypress.io/desktop/5.3.0?platform=linux&arch=x64
Error: Failed downloading the Cypress binary.
Response code: 502
Response message: Bad Gateway

Re-running it should probably fix that. Hopefully the GitHub Actions infrastructure problem is only short lived today...


That failure of the frontend-lint tests was indeed just a temporary thing and hasn't happened again. This PR is showing a (single) failure in the backend-lint tests though:

=========================== short test summary info ============================
FAILED tests/test_migrations.py::test_only_single_head_revision_in_migrations
========== 1 failed, 735 passed, 65480 warnings in 246.71s (0:04:06) ===========

Looking at the details for that failure, it has:

_________________ test_only_single_head_revision_in_migrations _________________

self = <alembic.script.base.ScriptDirectory object at 0x7f0502fb13d0>
ancestor = None
multiple_heads = 'The script directory has multiple heads (due to branching).Please use get_heads(), or merge the branches using alembic merge.'
start = None, end = None, resolution = None

    @contextmanager
    def _catch_revision_errors(
        self,
        ancestor: Optional[str] = None,
        multiple_heads: Optional[str] = None,
        start: Optional[str] = None,
        end: Optional[str] = None,
        resolution: Optional[str] = None,
    ) -> Iterator[None]:
        try:
>           yield

/usr/local/lib/python3.8/site-packages/alembic/script/base.py:246: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/local/lib/python3.8/site-packages/alembic/script/base.py:395: in get_current_head
    return self.revision_map.get_current_head()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <alembic.script.revision.RevisionMap object at 0x7f0502fb1370>
branch_label = None

    def get_current_head(
        self, branch_label: Optional[str] = None
    ) -> Optional[str]:
        """Return the current head revision.
    
        If the script directory has multiple heads
        due to branching, an error is raised;
        :meth:`.ScriptDirectory.get_heads` should be
        preferred.
    
        :param branch_label: optional branch name which will limit the
         heads considered to those which include that branch_label.
    
        :return: a string revision number.
    
        .. seealso::
    
            :meth:`.ScriptDirectory.get_heads`
    
        """
        current_heads: Sequence[str] = self.heads
        if branch_label:
            current_heads = self.filter_for_lineage(
                current_heads, branch_label
            )
        if len(current_heads) > 1:
>           raise MultipleHeads(
                current_heads,
                "%s@head" % branch_label if branch_label else "head",
            )
E           alembic.script.revision.MultipleHeads: Multiple heads are present for given argument 'head'; 9aafdb8de771, fd4fc850d7ea

/usr/local/lib/python3.8/site-packages/alembic/script/revision.py:491: MultipleHeads

The above exception was the direct cause of the following exception:

    def test_only_single_head_revision_in_migrations():
        """
        If multiple developers are working on migrations and one of them is merged before the
        other you might end up with multiple heads (multiple revisions with the same down_revision).
    
        This makes sure that there is only a single head revision in the migrations directory.
    
        Adopted from [https://blog.jerrycodes.com/multiple-heads-in-alembic-migrations/.](https://blog.jerrycodes.com/multiple-heads-in-alembic-migrations/)
        """
        config = Config(os.path.join("migrations", "alembic.ini"))
        config.set_main_option("script_location", "migrations")
        script = ScriptDirectory.from_config(config)
    
        # This will raise if there are multiple heads
>       script.get_current_head()

tests/test_migrations.py:21: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
/usr/local/lib/python3.8/site-packages/alembic/script/base.py:395: in get_current_head
    return self.revision_map.get_current_head()
/usr/local/lib/python3.8/contextlib.py:131: in __exit__
    self.gen.throw(type, value, traceback)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <alembic.script.base.ScriptDirectory object at 0x7f0502fb13d0>
ancestor = None
multiple_heads = 'The script directory has multiple heads (due to branching).Please use get_heads(), or merge the branches using alembic merge.'
start = None, end = None, resolution = None

    @contextmanager
    def _catch_revision_errors(
        self,
        ancestor: Optional[str] = None,
        multiple_heads: Optional[str] = None,
        start: Optional[str] = None,
        end: Optional[str] = None,
        resolution: Optional[str] = None,
    ) -> Iterator[None]:
        try:
            yield
        except revision.RangeNotAncestorError as rna:
            if start is None:
                start = cast(Any, rna.lower)
            if end is None:
                end = cast(Any, rna.upper)
            if not ancestor:
                ancestor = (
                    "Requested range %(start)s:%(end)s does not refer to "
                    "ancestor/descendant revisions along the same branch"
                )
            ancestor = ancestor % {"start": start, "end": end}
            raise util.CommandError(ancestor) from rna
        except revision.MultipleHeads as mh:
            if not multiple_heads:
                multiple_heads = (
                    "Multiple head revisions are present for given "
                    "argument '%(head_arg)s'; please "
                    "specify a specific target revision, "
                    "'<branchname>@%(head_arg)s' to "
                    "narrow to a specific head, or 'heads' for all heads"
                )
            multiple_heads = multiple_heads % {
                "head_arg": end or mh.argument,
                "heads": util.format_as_comma(mh.heads),
            }
>           raise util.CommandError(multiple_heads) from mh
E           alembic.util.exc.CommandError: The script directory has multiple heads (due to branching).Please use get_heads(), or merge the branches using alembic merge.

/usr/local/lib/python3.8/site-packages/alembic/script/base.py:272: CommandError

I'm not really sure what to make of it. 😦


@osule If you're interested in taking a look at this, we recently re-created new instructions on how to set up a local development environment + test things:

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

We know for sure those instructions works, as we use them pretty much daily. 😄

@osule
Copy link
Contributor Author

osule commented Jul 16, 2023

@justinclift Thanks for trying to get this.
I'll take a look at the information you have shared. I'll regenerate the migration against the latest version.

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.

Fixes getredash#2137
@osule osule force-pushed the 2137-case-insensitive-result-cache branch from 3e20d24 to 59e6099 Compare July 16, 2023 21:15
@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

Merging #4254 (59e6099) into master (02d128e) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4254   +/-   ##
=======================================
  Coverage   59.71%   59.71%           
=======================================
  Files         151      151           
  Lines       12261    12261           
  Branches     1658     1658           
=======================================
  Hits         7322     7322           
  Misses       4733     4733           
  Partials      206      206           
Impacted Files Coverage Δ
redash/utils/__init__.py 71.09% <100.00%> (ø)

@justinclift
Copy link
Member

Thanks heaps for fixing that CI failure @osule. Merging this now. 😄

@justinclift justinclift merged commit c8516d3 into getredash:master Jul 17, 2023
EmilaineBorato pushed a commit to sondautilities/sonda_d4b_redash that referenced this pull request Jul 24, 2023
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.

Fixes getredash#2137
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.

Case insensitive parameters in Redash query result cache
4 participants