Added Support for default prefixes in SPARQL queries (#647)#787
Added Support for default prefixes in SPARQL queries (#647)#787Aeternant wants to merge 1 commit into
Conversation
mmarx
left a comment
mmarx
left a comment
There was a problem hiding this comment.
This is mostly fine, only a few stylistic issues remain
| base: Option<Iri<String>>, | ||
| prefixes: HashMap<String, IriRef<String>>, |
There was a problem hiding this comment.
This is fine for now, but we would want to consider moving these into a FormatContext (or something like that) struct if we ever add more parameters
| ( | ||
| key.clone(), | ||
| Iri::from_str(&value.to_string()) | ||
| .unwrap_or_else(|_| base.clone().unwrap().resolve(value).unwrap()), |
There was a problem hiding this comment.
We generally avoid unwrap() in favour of expect() (this is somewhat less strict in tests, but in library code, we don't want to have any plain unwraps).
Here, I don't even think the unwrap on base is warranted – it might be None and that could still be valid if the relative prefix isn't actually used in the query. The validation should proceed like this:
- if
baseisNone, only collect the absolute prefixes (if the prefix is required in the query, the existing error handling will already catch it, if not, this is fine) - otherwise, collect all the prefixes, resolving relative prefixes with respect to the base (this will always succeed, so you can
base.resolve(value).expect("relative IRI can always be resolved with the absolute base")here
let resolved_prefixes = if let Some(base) = base {
prefixes.iter().map(|(key, value)| (key.clone(), Iri::from_str(value).unwrap_or_else(|_| base.resolve(value).expect("relative IRI can always be resolved with absolute base")).collect()
} else {
prefixes.iter().filter_map(|(key, value)| (key.clone(), Iri::from_str(value)?).collect()
}| .collect(); | ||
|
|
||
| if let Some(base) = base { | ||
| parser = parser.with_base_iri(base.to_string()).unwrap(); |
There was a problem hiding this comment.
| parser = parser.with_base_iri(base.to_string()).unwrap(); | |
| parser = parser.with_base_iri(base.to_string()).expect("IRI is valid"); |
| for (prefix_name, prefix_iri) in resolved_prefixes { | ||
| parser = parser | ||
| .with_prefix(prefix_name, prefix_iri.to_string()) | ||
| .unwrap(); |
There was a problem hiding this comment.
(see above, also applies elsewhere in the PR)
| let mut parser = SparqlParser::new(); | ||
|
|
||
| // SparqlParser only accepts absolute (resolved) iri's | ||
| let resolved_prefixes: Vec<(String, Iri<String>)> = prefixes | ||
| .iter() | ||
| .map(|(key, value)| { | ||
| ( | ||
| key.clone(), | ||
| Iri::from_str(&value.to_string()) | ||
| .unwrap_or_else(|_| base.clone().unwrap().resolve(value).unwrap()), | ||
| ) | ||
| }) | ||
| .collect(); | ||
|
|
||
| if let Some(base) = base { | ||
| parser = parser.with_base_iri(base.to_string()).unwrap(); | ||
| } | ||
|
|
||
| for (prefix_name, prefix_iri) in resolved_prefixes { | ||
| parser = parser | ||
| .with_prefix(prefix_name, prefix_iri.to_string()) | ||
| .unwrap(); | ||
| } |
There was a problem hiding this comment.
It's maybe worthwhile to factor this into function that takes base and prefixes and returns a SparqlParser, since we're doing the same thing above
|
|
||
| let valid_query = AnyDataValue::new_plain_string(String::from(" | ||
| PREFIX wikibase: <http://wikiba.se/ontology#> | ||
| PREFIX wikibase: <http://wikiba.se/ontology#> |
There was a problem hiding this comment.
Not sure why we change the alignment here?
| let parameters = Parameters::<B>::validate( | ||
| predicate, | ||
| spec, | ||
| bindings, | ||
| filter_rules, | ||
| direction, | ||
| report, | ||
| base.clone(), | ||
| prefixes.clone(), | ||
| )?; |
There was a problem hiding this comment.
This should probably be factored into one or possible several (that'd make it easier to reuse them elsewhere) structs. We can discuss the details in person tomorrow.
| let base = translation | ||
| .base | ||
| .clone() | ||
| .map(|x| Iri::from_str(&x.0).unwrap()); |
There was a problem hiding this comment.
Is the base already validated at this point? If yes, we can expect here, otherwise, this might need to produce an error.
| let prefixes = translation | ||
| .prefix_mapping | ||
| .iter() | ||
| .map(|(key, value)| (key.clone(), IriRef::from_str(&value.0).unwrap())) |
There was a problem hiding this comment.
Same question as for the base above.
2 participants
Makes base and prefix IRIs available to Import/Export and closes #647