Malformed SPARQL on Batch when XACML is disabled

DiegoPino commented 4 years ago

Hi,

If XACML module is disabled and, e.g, a Book is removed from the repository, IW triggers a sub batch to clear any possible Orphan Children. To fetch the list of Children it executes a SPARQL query, which ends being malformed and triggering a 500 error when send via Tuque.

A . before the OPTIONAL statement is the culprit since, without XAML optionals, it ends concatenating 2x dots in a row, which is invalid SPARQL.

This error is quite annoying since it breaks the running Batch (the main, core, Book deletion) leaving the user without enough information to react and forcing them to go and have a bitter coffee in despair.

This happens here

https://github.com/Islandora-Collaboration-Group/islandora_webform/blob/99bc00c58c0c08bd1f020afb87787f9634cc387e/submodules/islandora_webform_ingest/includes/utilities.inc#L881

Malfored SPARQL Query

SELECT ?object ?predicate
FROM <#ri>
WHERE {
  ?object <fedora-model:label> ?title ;
  ?predicate <info:fedora/someobject:2> .

  . OPTIONAL { ?object2 <fedora-model:label> ?title ; ?predicate <info:fedora/someobject:2>. ?object2 <fedora-rels-ext:isMemberOfCollection> ?collection}
  FILTER( !sameTerm(?predicate, <fedora-rels-ext:isMemberOfCollection>) && !sameTerm(?predicate, <fedora-rels-ext:isMemberOf>) && !sameTerm(?predicate, <fedora-model:hasModel>)) FILTER(!bound(?collection))
}

Correct SPARQL Query (don't mind the blank line there...)

SELECT ?object ?predicate
FROM <#ri>
WHERE {
  ?object <fedora-model:label> ?title ;
  ?predicate <info:fedora/someobject:2> .

  OPTIONAL { ?object2 <fedora-model:label> ?title ; ?predicate <info:fedora/someobject:2>. ?object2 <fedora-rels-ext:isMemberOfCollection> ?collection} .
  FILTER( !sameTerm(?predicate, <fedora-rels-ext:isMemberOfCollection>) && !sameTerm(?predicate, <fedora-rels-ext:isMemberOf>) && !sameTerm(?predicate, <fedora-model:hasModel>)) FILTER(!bound(?collection))
}

Solution is to replace current OPTIONAL string formatters for this

$query = format_string($query, array(
    '!optionals' => !empty($query_optionals) ? ('OPTIONAL {{' . implode('} UNION {', $query_optionals) . '}} . ') : '',
    '!filters' => implode(' ', array_map($filter_map, $query_filters)),
  ));
  $query = format_string($query, array(
    '!optexclude' => !empty($query_unbound_optionals) ? ('OPTIONAL { ' . $query_unbound_optionals . '} .') : '',
  ));

noahwsmith commented 4 years ago

Thank you for this, @DiegoPino !

I'll see if I can free up some time from either @patdunlavey or @emudojo to test and merge this (cc @dmer )

DiegoPino commented 4 years ago

@noahwsmith thanks!

noahwsmith commented 4 years ago

@DiegoPino I tested this on our LASIR_DEMO server and it seems fine. Can you confirm that Pat's PR works for you?

DiegoPino commented 4 years ago

@noahwsmith thanks!! @patdunlavey thanks also =) can check tomorrow morning, will let you know.

DiegoPino commented 4 years ago

@noahwsmith this looks Ok, sorry for the delay. Its a 1:1 of what i suggested and i tested that in production. Thansk

noahwsmith commented 4 years ago

Thanks @DiegoPino !

@dwk2 I don't have merge ability on this repo, but I recommend accepting the PR to address the bug Diego raised.

dwk2 commented 4 years ago

Thank you @patdunlavey , @DiegoPino and @noahwsmith ! Closing issue.

Islandora-Collaboration-Group / islandora_webform