Skip to content

Metadata field: retrieve by exact name#145

Merged
tdonohue merged 2 commits intoDSpace:mainfrom
atmire:metadata-field-exact-name
Aug 6, 2020
Merged

Metadata field: retrieve by exact name#145
tdonohue merged 2 commits intoDSpace:mainfrom
atmire:metadata-field-exact-name

Conversation

@benbosman
Copy link
Copy Markdown
Member

This is related to DSpace/DSpace#2899 (comment)

For the front end validation on whether or not a filled in mdField is valid (on the edit item > metadata page) an exact retrieval of a mdField is needed, where either exactly one matching mdField is given or none if it doesn’t exist.

The findByName endpoint is far from ideal for this, it would have the drawbacks:

  • For mdFields without a qualifier, it would need to be handled separately to determine there's no qualifier
  • In frontend, it would need to be specified there's no qualifier
  • Based the search results, the field has to be searched for, and no match implies it doesn't exist

For these use cases, the recommended Spring REST solution would be to create a separate non-search endpoint, retrieving the field based on the unique alternative ID, and responding with 404 if it doesn't exist

@benbosman benbosman added this to the 7.0beta4 milestone Jul 29, 2020
@tdonohue
Copy link
Copy Markdown
Member

@benbosman : Why couldn't we just add a new exact param to the byFieldName which would work like:
/api/core/metadatafields/search/byFieldName?exact=dc.title

That'd only return 200 (and one result) if there is an exact field named "dc.title" (in this case no qualifier), and 404 if it doesn't exist. When this param is used, it could even skip querying Solr, and just use metadataFieldService.findByString(context, mdFieldName, '.'); directly.

The reason I ask is that this new endpoint seems like an exact search. Implementing it this way would also allow us to handle it in the MetadataFieldRestRepository instead of requiring an entirely separate endpoint with a separate Controller class....so, it should also be less code overall.

While we could have a separate endpoint, it seems rather confusing to have two endpoints doing very similar searching by name (as it could be confusing to others which one to use in each scenario). It also seems like there should be a way to simply have byFieldName perform an exact field search.

@benbosman
Copy link
Copy Markdown
Member Author

The primary reason to avoid the byFieldName is because that's a search.

The use case where you have a unique name would be cleaner to support without a search. This ensures:

  • There's no irrelevant paging, list of fields, …
  • If there's no result, it's a 404, which is much easier in Angular to work with compared to an empty search result

This is similar to https://stackoverflow.com/questions/20381976/rest-api-design-getting-a-resource-through-rest-with-different-parameters-but/20386425#20386425

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Jul 29, 2020

@benbosman : You are correct that it's a perfectly valid way to design a REST API. However, it doesn't align with the Spring Data REST design best practices for an "Item resource" (single resource), where it always expects a single unique identifier (i.e. /objects/{id}): https://docs.spring.io/spring-data/rest/docs/current/reference/html/#repository-resources.item-resource

Additionally, I'm not sure how this would look in a HAL document (it's likely it simply wouldn't appear there by default, so the UI would have to know it existed, instead of being able to find it via HAL links).

I'm looking around a bit to see if there's a different way of doing this in Spring Data REST, but I wanted to simply note that I'm concerned that this design doesn't align with our documented best practices in our README, as we don't document a way to "lookup" single resources in this manner.

@tdonohue
Copy link
Copy Markdown
Member

tdonohue commented Jul 29, 2020

@benbosman : Just as a follow-up, I've discovered we do have other endpoints which specify they use a search to return a single unique object / resource.

One example is: /api/submission/workspaceitems/search/item?uuid=<:item-uuid> https://github.com/DSpace/Rest7Contract/blob/main/workspaceitems.md#get-single-workspace-item-from-item-uuid
(WorkflowItem also has this same search)

Another example is: /api/eperson/eperson/search/byEmail?email=<:email> https://github.com/DSpace/DSpace/blob/main/dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java#L249

Digging back in GitHub, I found we had a very similar discussion around this back in the Versioning Rest Contract PR, where @abollini pointed out this type of unique lookup (via a non-ID) should be a search returning 0 or 1. In that PR, I had also suggested we need to be using searches for anything that is NOT a the primary ID of the resource. See: #97 (comment)

So, I still think the best direction here is to follow the same path we've done in the past...move this to a search that returns either 0 or 1. My guess would be that the UI should already know how to handle that sort of unique search, as it must be somehow handling it for the example endpoints noted above.

@tdonohue tdonohue self-requested a review July 30, 2020 14:50
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

Just officially submitting my review. As noted in the comments above, I think this should be changed to use the /search/byFieldName endpoint, or a new /search/byExactFieldName endpoint (if you'd rather it be a separate path). Either way, like the above examples (see last comment), it should return either 0 or 1 result, based on whether that exact metadata field exists or not.

@benbosman benbosman requested a review from tdonohue August 6, 2020 13:53
@tdonohue tdonohue added 1 APPROVAL pull request only requires a single approval to merge. high priority labels Aug 6, 2020
Copy link
Copy Markdown
Member

@tdonohue tdonohue left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me now. Thanks @benbosman !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 APPROVAL pull request only requires a single approval to merge. high priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants