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

[DS-4010] Removed the escaping on the query parameter for the discove… #2206

Merged
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -142,7 +142,8 @@ public SearchResultsResource getSearchObjects(@RequestParam(name = "query", requ
}

//Get the Search results in JSON format
SearchResultsRest searchResultsRest = discoveryRestRepository
SearchResultsRest searchResultsRest = null;
searchResultsRest = discoveryRestRepository
.getSearchObjects(query, dsoType, dsoScope, configurationName, searchFilters, page);

//Convert the Search JSON results to paginated HAL resources
Expand Down
Expand Up @@ -38,6 +38,7 @@
*/
@ControllerAdvice
public class DSpaceApiExceptionControllerAdvice extends ResponseEntityExceptionHandler {

@Autowired
private RestAuthenticationService restAuthenticationService;

Expand All @@ -51,6 +52,12 @@ protected void handleAuthorizeException(HttpServletRequest request, HttpServletR
}
}

@ExceptionHandler(IllegalArgumentException.class)
protected void handleIllegalArgumentException(HttpServletRequest request, HttpServletResponse response,
Exception ex) throws IOException {
sendErrorResponse(request, response, ex, ex.getMessage(), HttpServletResponse.SC_BAD_REQUEST);
}

@ExceptionHandler(SQLException.class)
protected void handleSQLException(HttpServletRequest request, HttpServletResponse response, Exception ex)
throws IOException {
Expand Down
Expand Up @@ -9,6 +9,8 @@

import java.util.List;

import javax.ws.rs.BadRequestException;

import org.apache.log4j.Logger;
import org.dspace.app.rest.converter.DiscoverConfigurationConverter;
import org.dspace.app.rest.converter.DiscoverFacetConfigurationConverter;
Expand Down Expand Up @@ -91,7 +93,7 @@ public SearchConfigurationRest getSearchConfiguration(final String dsoScope, fin
public SearchResultsRest getSearchObjects(final String query, final String dsoType, final String dsoScope,
final String configurationName,
final List<SearchFilter> searchFilters, final Pageable page)
throws InvalidRequestException {
throws InvalidRequestException, BadRequestException {
Context context = obtainContext();

DSpaceObject scopeObject = scopeResolver.resolveScope(context, dsoScope);
Expand All @@ -108,6 +110,7 @@ public SearchResultsRest getSearchObjects(final String query, final String dsoTy

} catch (SearchServiceException e) {
log.error("Error while searching with Discovery", e);
throw new IllegalArgumentException();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe change this to new IllegalArgumentException("Error while searching with Discovery: " + e.getMessage()); or new IllegalArgumentException("Error while searching with Discovery, this might be a query syntax exception"); to make clients aware that we use a specific query syntax.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also make sure that that message reaches the client (e.g. HAL Browser)?

Copy link
Contributor

Choose a reason for hiding this comment

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

I now also realize that Warning: (un)escaped characters ahead! has probably a valid syntax. But it would not return the item with title "Warning: (un)escaped characters ahead!".

Can you maybe also add an additional test query call for the "Faithful Infidel: Exploring Conformity (2nd edition)" test item but without escaping (thus query Faithful Infidel: Exploring Conformity (2nd edition))? My guess is that that query returns 0 results.

Copy link
Member

Choose a reason for hiding this comment

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

As @tomdesair noted here, this Exception should not be empty, as it provides no information to the user (and "swallows" the actual error). Please update this to be: new IllegalArgumentException("Error while searching with Discovery: " + e.getMessage()); or similar.

Copy link
Member

Choose a reason for hiding this comment

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

@tdonohue: @Raf-atmire just committed this modification

}

return discoverResultConverter
Expand Down
Expand Up @@ -181,9 +181,7 @@ private DiscoverQuery buildCommonDiscoverQuery(Context context, DiscoveryConfigu

//Set search query
if (StringUtils.isNotBlank(query)) {
//Note that these quotes are needed incase we try to query OR for example.
//If the quotes aren't present, it'll crash.
queryArgs.setQuery("\"" + searchService.escapeQueryChars(query) + "\"");
queryArgs.setQuery(query);
}

//Limit results to DSO type
Expand Down
Expand Up @@ -2678,4 +2678,99 @@ public void discoverSearchObjectsWithQueryOperatorNotAuthority() throws Exceptio
;

}

@Test
public void discoverSearchObjectsTestWithDateIssuedQuery() throws Exception {
//We turn off the authorization system in order to create the structure as defined below
context.turnOffAuthorisationSystem();

//** GIVEN **
//1. A community-collection structure with one parent community with sub-community and two collections.
parentCommunity = CommunityBuilder.createCommunity(context)
.withName("Parent Community")
.build();
Community child1 = CommunityBuilder.createSubCommunity(context, parentCommunity)
.withName("Sub Community")
.build();
Collection col1 = CollectionBuilder.createCollection(context, child1).withName("Collection 1").build();
Collection col2 = CollectionBuilder.createCollection(context, child1).withName("Collection 2").build();

//2. Three public items that are readable by Anonymous with different subjects
Item publicItem1 = ItemBuilder.createItem(context, col1)
.withTitle("Test")
.withIssueDate("2010-10-17")
.withAuthor("Smith, Donald")
.withSubject("ExtraEntry")
.build();

Item publicItem2 = ItemBuilder.createItem(context, col2)
.withTitle("Test 2")
.withIssueDate("1990-02-13")
.withAuthor("Smith, Maria").withAuthor("Doe, Jane")
.withSubject("TestingForMore").withSubject("ExtraEntry")
.build();

Item publicItem3 = ItemBuilder.createItem(context, col2)
.withTitle("Public item 2")
.withIssueDate("2010-02-13")
.withAuthor("Smith, Maria").withAuthor("Doe, Jane").withAuthor("test,test")
.withAuthor("test2, test2").withAuthor("Maybe, Maybe")
.withSubject("AnotherTest").withSubject("TestingForMore")
.withSubject("ExtraEntry")
.build();
String bitstreamContent = "ThisIsSomeDummyText";
//Add a bitstream to an item
try (InputStream is = IOUtils.toInputStream(bitstreamContent, CharEncoding.UTF_8)) {
Bitstream bitstream = BitstreamBuilder.
createBitstream(context, publicItem1, is)
.withName("Bitstream")
.withMimeType("text/plain")
.build();
}

//Run the filter media to make the text in the bitstream searchable through the query
runDSpaceScript("filter-media", "-f", "-i", publicItem1.getHandle());

//** WHEN **
getClient().perform(get("/api/discover/search/objects")
.param("query", "dc.date.issued:2010-02-13"))

//** THEN **
//The status has to be 200 OK
.andExpect(status().isOk())
//The type has to be 'discover'
.andExpect(jsonPath("$.type", is("discover")))
//The page object needs to look like this
.andExpect(jsonPath("$._embedded.searchResult.page", is(
PageMatcher.pageEntryWithTotalPagesAndElements(0, 20, 1, 1)
)))
//This is the only item that should be returned with the query given
.andExpect(jsonPath("$._embedded.searchResult._embedded.objects", Matchers.contains(
SearchResultMatcher.matchOnItemName("item", "items", "Public item 2")
)))

//There always needs to be a self link available
.andExpect(jsonPath("$._links.self.href", containsString("/api/discover/search/objects")))
;


//** WHEN **
getClient().perform(get("/api/discover/search/objects")
.param("query", "dc.date.issued:2013-02-13"))

//** THEN **
//The status has to be 200 OK
.andExpect(status().isOk())
//The type has to be 'discover'
.andExpect(jsonPath("$.type", is("discover")))
//The page object needs to look like this
.andExpect(jsonPath("$._embedded.searchResult.page", is(
PageMatcher.pageEntryWithTotalPagesAndElements(0, 20, 0, 0)
)))

//There always needs to be a self link available
.andExpect(jsonPath("$._links.self.href", containsString("/api/discover/search/objects")))
;

}
}
Expand Up @@ -168,7 +168,7 @@ public void testBuildQuery() throws Exception {
Arrays.asList(searchFilter), "item", page);

assertThat(discoverQuery.getFilterQueries(), containsInAnyOrder("archived:true", "subject:\"Java\""));
assertThat(discoverQuery.getQuery(), is("\"" + query + "\""));
assertThat(discoverQuery.getQuery(), is(query));
assertThat(discoverQuery.getDSpaceObjectFilter(), is(Constants.ITEM));
assertThat(discoverQuery.getSortField(), is("dc.title_sort"));
assertThat(discoverQuery.getSortOrder(), is(DiscoverQuery.SORT_ORDER.asc));
Expand Down Expand Up @@ -293,7 +293,7 @@ public void testBuildFacetQuery() throws Exception {
"subject");

assertThat(discoverQuery.getFilterQueries(), containsInAnyOrder("archived:true", "subject:\"Java\""));
assertThat(discoverQuery.getQuery(), is("\"" + query + "\""));
assertThat(discoverQuery.getQuery(), is(query));
assertThat(discoverQuery.getDSpaceObjectFilter(), is(Constants.ITEM));
assertThat(discoverQuery.getSortField(), isEmptyOrNullString());
assertThat(discoverQuery.getMaxResults(), is(0));
Expand Down