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
Collaborator

@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 Jan 19, 2021
- Add links to Gravsearch article.
@benjamingeer benjamingeer requested a review from SepidehAlassi Jan 19, 2021
@benjamingeer benjamingeer marked this pull request as ready for review Jan 19, 2021
@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Jan 21, 2021

@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

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:

This comment has been minimized.

@SepidehAlassi

SepidehAlassi Jan 22, 2021
Contributor

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

@benjamingeer
Copy link
Collaborator Author

@benjamingeer benjamingeer commented Jan 25, 2021

Thanks for reviewing!

@benjamingeer benjamingeer merged commit 48d77cd into main Jan 25, 2021
10 checks passed
10 checks passed
Build Everything
Details
Prepare next release
Details
API Unit Tests
Details
API E2E Tests
Details
API Integration Tests
Details
Upgrade Integration Tests
Details
Docs Build Test
Details
Publish (on release only)
Details
Deploy docs (on release only)
Details
Google chat notification about release and published version
Details
@benjamingeer benjamingeer deleted the fix/DSP-1240-union-scope branch Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants