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!(gravsearch): Handle UNION scopes with FILTER correctly (DSP-1240) #1790

Merged
merged 8 commits into from Jan 25, 2021

Conversation

benjamingeer
Copy link

@benjamingeer benjamingeer commented Jan 19, 2021

resolves DSP-1240

  • Keep track of UNION scopes separately for generated variables.
  • In a UNION block, don't allow a FILTER that refers to a variable that hasn't been bound in the same block.
  • If a variable is used in ORDER BY, require it to be bound at the top level of the WHERE clause.
  • Add tests.
  • Add docs.

Breaking Changes

FILTER in UNION

A FILTER in a UNION block can only use variables that are bound in the same block, otherwise the query will be rejected. This query is invalid because ?text is not bound in the UNION block containing the FILTER where the variable is used:

PREFIX knora-api: <http://api.knora.org/ontology/knora-api/simple/v2#>
PREFIX mls: <http://0.0.0.0:3333/ontology/0807/mls/simple/v2#>

CONSTRUCT {
    ?lemma knora-api:isMainResource true .
    ?lemma mls:hasLemmaText ?text .        
} WHERE {
    ?lemma a mls:Lemma .
    ?lemma mls:hasLemmaText ?text .
    
    {
        ?lemma mls:hasPseudonym ?pseudo .
        FILTER regex(?pseudo, "Abel", "i") .
    } UNION {
        FILTER regex(?text, "Abel", "i") .
    }
}
ORDER BY ASC(?text)
OFFSET 0

It can be corrected like this:

PREFIX knora-api: <http://api.knora.org/ontology/knora-api/simple/v2#>
PREFIX mls: <http://0.0.0.0:3333/ontology/0807/mls/simple/v2#>

CONSTRUCT {
    ?lemma knora-api:isMainResource true .
    ?lemma mls:hasLemmaText ?text .        
} WHERE {
    ?lemma a mls:Lemma .
    ?lemma mls:hasLemmaText ?text .
    
    {
        ?lemma mls:hasPseudonym ?pseudo .
        FILTER regex(?pseudo, "Abel", "i") .
    } UNION {
        ?lemma mls:hasLemmaText ?text .
        FILTER regex(?text, "Abel", "i") .
    }
}
ORDER BY ASC(?text)
OFFSET 0

ORDER BY

A variable used in ORDER BY must be bound at the top level of the WHERE clause. This query is invalid, because ?int is not bound at the top level of the WHERE clause:

PREFIX knora-api: <http://api.knora.org/ontology/knora-api/v2#>
PREFIX anything: <http://0.0.0.0:3333/ontology/0001/anything/v2#>

CONSTRUCT {
    ?thing knora-api:isMainResource true .
    ?thing anything:hasInteger ?int .
    ?thing anything:hasRichtext ?richtext .
    ?thing anything:hasText ?text .
} WHERE {
    ?thing a knora-api:Resource .
    ?thing a anything:Thing .

    {
        ?thing anything:hasRichtext ?richtext .
        FILTER knora-api:matchText(?richtext, "test")
        ?thing anything:hasInteger ?int .
    }
    UNION
    {
        ?thing anything:hasText ?text .
        FILTER knora-api:matchText(?text, "test")
	?thing anything:hasInteger ?int .
    }
}
ORDER BY (?int)

It can be corrected like this:

PREFIX knora-api: <http://api.knora.org/ontology/knora-api/v2#>
PREFIX anything: <http://0.0.0.0:3333/ontology/0001/anything/v2#>

CONSTRUCT {
    ?thing knora-api:isMainResource true .
    ?thing anything:hasInteger ?int .
    ?thing anything:hasRichtext ?richtext .
    ?thing anything:hasText ?text .
} WHERE {
    ?thing a knora-api:Resource .
    ?thing a anything:Thing .
    ?thing anything:hasInteger ?int .

    {
        ?thing anything:hasRichtext ?richtext .
        FILTER knora-api:matchText(?richtext, "test")
    }
    UNION
    {
        ?thing anything:hasText ?text .
        FILTER knora-api:matchText(?text, "test")
    }
}
ORDER BY (?int)

@benjamingeer benjamingeer self-assigned this Jan 19, 2021
@benjamingeer benjamingeer marked this pull request as draft January 19, 2021 13:38
@benjamingeer benjamingeer added API/V2 bug something isn't working Gravsearch breaking any breaking change and removed breaking any breaking change labels Jan 19, 2021
Benjamin Geer added 2 commits January 19, 2021 16:35
@benjamingeer benjamingeer marked this pull request as ready for review January 19, 2021 15:49
@benjamingeer
Copy link
Author

@loicjaouen This may affect you; the ORDER BY example above comes from a test case you wrote for another issue.

Copy link
Contributor

@SepidehAlassi SepidehAlassi left a comment

Choose a reason for hiding this comment

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

This looks good, thanks.


### FILTER in UNION

A `FILTER` in a `UNION` block can only use variables that are bound in the same block, otherwise the query will be rejected. This query is invalid because `?text` is not bound in the `UNION` block containing the `FILTER` where the variable is used:
Copy link
Contributor

Choose a reason for hiding this comment

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

ooh, even I did not know that. Thanks for the examples.

@benjamingeer
Copy link
Author

Thanks for reviewing!

@benjamingeer benjamingeer merged commit 48d77cd into main Jan 25, 2021
@benjamingeer benjamingeer deleted the fix/DSP-1240-union-scope branch January 25, 2021 09:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API/V2 breaking any breaking change bug something isn't working Gravsearch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants