DSpace / RestContract

REST Contract for DSpace 7-8
https://wiki.lyrasis.org/display/DSDOC8x/
37 stars 48 forks source link

[DS-4301] Added Content Reports section and Filtered Collections report therein #202

Closed jeffmorin closed 6 months ago

jeffmorin commented 1 year ago

This pull request features a REST specification for the first of the two REST-based reports to be ported to DSpace 7.x (see ticket DS-4301, https://github.com/DSpace/DSpace/issues/7641), the Filtered Collections report.

Following a suggestion from Andrea Bollini, I created a new /contentreports branch in which I created a /filteredcollections URI for the report itself. I added an entry under Endpoints Under Development/Discussion in endpoints.md.

I also added a missing link and removed a duplicate entry in endpoints.md.

jeffmorin commented 1 year ago

Added the second report, Filtered Items (for metadata queries).

jeffmorin commented 1 year ago

I changed all contentreports occurrences to contentreport to address a request in the REST layer regarding category names.

paulo-graca commented 1 year ago

Thank you @jeffmorin for this important contribution. I would like to help you bringing this to DSpace core. I didn't (yet) check your implementation and I was expecting to have the DSpace demo site working for comparing, but it's unavailable.

I agree with @tdonohue , our Rest Contract defines that POST method should be used to create new resources, leaving the GET to access resources: https://github.com/DSpace/RestContract#on-collection-of-resources-endpoints

My understanting it's that we aren't creating resources, but access them, so we should use GET. I understand that we have a GET max chars limitation and a POST could help us in this matter. That's why this solution. A somehow similiar feature (to me) was addressed at: https://github.com/DSpace/RestContract/blob/main/search-endpoint.md

At the discovery or search endpoints, a complex structure is defined at the backend, stating what the client can do (defining available browses, facets, sorts options,...). It's also possible use query string to pass params. But, when pre-defining what structure to use, you don't need to pass a lot of query params. I didn't saw (yet) your implementation, but this feature could somehow benefict from discovery's rest design.

mwoodiupui commented 1 year ago

I could argue that we are neither creating nor accessing resources, because these reports are not resources; they are snapshots of the operational state of a store of resources. (I realize that this is the opposite of what I said above.) What is wanted is really not REST at all, but REST is all that we have to work with. REST basically defines putting things into a store and getting them out again. Complex configurable reports on the status of the store itself are something that REST isn't able to do if one cares about ideological purity.

The notion of triggering pre-defined reports (which is how I understand the Discovery analogy) is interesting, but I think that we need to consider how the reports will be used. Statistics might be a useful model. One sometimes doesn't yet know what questions to ask, so one engages in wide-ranging exploration of the data to find promising perspectives. At other times (such as after exploratory examination) one has specific questions to be answered, possibly over and over again as the dataset evolves. Is this a good fit to the way we expect Content Reports to be used? Because doing exploratory work by repeatedly editing a set of preconfigured reports on the back-end sounds slow, tedious and exhausting.

paulo-graca commented 1 year ago

I don't have access to this feature right now, but I found a very useful video by Terry Brady (@terrywbrady ) https://www.youtube.com/watch?v=K2gGHYUZI40 and he shows some work they did and how we can interact with it.

Just to contextualize others, I would also like to add the Wiki page URL for the DS6 feature: https://wiki.lyrasis.org/display/DSDOC6x/REST+Reports+-+Summary+of+API+Calls

jeffmorin commented 1 year ago

I just added a GET endpoint to the Filtered Items content report. I also made a minor update to the Filtered Collections report documentation.

jeffmorin commented 1 year ago

Please see the comment I wrote at https://github.com/DSpace/DSpace/pull/8598.

jeffmorin commented 9 months ago

My fork is now synchronized with the main branch.

mwoodiupui commented 6 months ago

Perhaps some of the difficulty is in calling it "beta"? That implies that the design is finished and the code is expected to work well. Would "experimental" or "preview" be more suitable? With a stern warning: "the work is unfinished and we expect to make substantial changes in the final, supported code to address, at least, these issues...."

Because that's what was agreed upon, I think: an early release in 8.0 despite 8.0's compressed schedule, to get this facility into the hands of users this year and to gather wider experience; and a stable release with full support by 9.0.

If people choose to rely on internals of code when they've been clearly warned not to, that is their problem, and we should not be shy about reminding them. Politely appreciative of feedback: yes; apologetic: no.

It would be helpful to have missing features logged as issues so that they can be studied, scheduled and tracked.

tdonohue commented 6 months ago

@paulo-graca : From talking with @pilasou and @jeffmorin , I don't think it's possible to update this PR any further in time for 8.0. The decision for 8.0 was to scale back because @pilasou and @jeffmorin don't have availability to do any more detailed work on this code. We have to remember that, while this is wanted by many, the development on this is entirely donated by U of Laval.

There are only two options that remain:

  1. We accept this work pretty much "as-is" for 8.0 based on the agreement we achieved in recent DevMtgs (see notes from Feb 8 meeting). Then we would work to improve it / fix bugs in 8.1+ and 9.0.
  2. OR, the only other option is to delay everything until 9.0. There is no time remaining to add more features or even design new endpoints....both of those take time, and we have no time left to get this into 8.0. (Keep in mind, this feature was initially built for 7.x, but was delayed until 8.0. This option would be yet another delay)

As decided in the meeting on Feb 8, we voted for option 1. That means we let this into 8.0 so that everyone can start to improve/enhance it. But, put warnings on it that it is missing features & there are some known issues. Those missing features / known issues can be detailed in GitHub issue tickets to be addressed in future 8.x or 9.0 releases.

paulo-graca commented 6 months ago

@tdonohue, to me, there are two different aspects here:

tdonohue commented 6 months ago

@paulo-graca : To clarify my point... I disagree with this statement:

Even if the implementation doesn't support it yet, I think those missing endpoints should be defined at this phase.

I feel the REST Contract should only define the endpoints that are implemented at this time. Otherwise, it is misleading to other developers as to what features actually exist. (And to clarify, we do have a versioned REST Contract. We have the main branch which is pre-8.0, and the dspace-7_x branch, which is the 7.x version of the REST Contract. So, the contract is versioned in the same way that the backend is versioned because the contract describes the backend.)

Overall, I agree that we should enhance this contract as necessary to describe the current implementation. However, we should not add endpoints which do not yet exist, as the Contract is meant to describe the implementation.

I hope that clarifies things. If you do have feedback on the contract compared to the current implementation, then please do point it out. We both agree that there is the opportunity to clean up this PR as the implementation PR also gets cleaned up.