Issue 1148 - Adding new Relationships in Edit Item is difficult for entities with many relationships

[benbosman]

References

Add references/links to any related issues or PRs. These may include:

• REST part of Adding new Relationships in Edit Item is difficult for entities with many relationships dspace-angular#1148
• Related to REST Contract

Description

This PR adds the new projections from DSpace/RestContract#165

As discussed in the previous community meeting, a PR has been created for this work to help clarify what it does.
This PR should clarify:

• The projections are calculating data
• There's no problem with the cache, because the HAL links are still unique

Instructions for Reviewers

I'm including some sample tests here:

CheckRelatedItem

/server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckRelatedItem&checkRelatedItem=8d60686a-cfad-42fb-88b0-33a482fa3346&embed=relationships

The main item contains:

"relatedItems": [
    "8d60686a-cfad-42fb-88b0-33a482fa3346"
  ]

For the links in the main item:

• The self link has the parameters:
  /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?checkRelatedItem=8d60686a-cfad-42fb-88b0-33a482fa3346&projection=CheckRelatedItem (ensuring the cache is still valid)

For the links in the relationships:

• The leftItem and rightItem have the parameters:
  /server/api/core/items/afa5e55d-0827-46dc-b292-2dbe10b40b97?checkRelatedItem=8d60686a-cfad-42fb-88b0-33a482fa3346&projection=CheckRelatedItem (ensuring the cache is still valid)

CheckSideItemInRelationship

/server/#/server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckSideItemInRelationship&checkSideItemInRelationship=a7e2ca60-5d0e-4d41-9457-9ab04702e18b&embed=relationships

For the links in the main item:

• The relationships link has the parameters:
  /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b/relationships?checkSideItemInRelationship=a7e2ca60-5d0e-4d41-9457-9ab04702e18b&projection=CheckSideItemInRelationship

For the links in the relationships:

• The self link has the parameters:
  /server/api/core/relationships/1305?checkSideItemInRelationship=a7e2ca60-5d0e-4d41-9457-9ab04702e18b&projection=CheckSideItemInRelationship

The relationship has the properties:

"relatedItemLeft": false,
"relatedItemRight": true,

[tdonohue]

@benbosman : Took a look at this today, and this implementation doesn't look RESTful to me, for the same reasons I've discussed in the Contract at DSpace/RestContract#165. Let me see if I can describe these reasons more clearly based on this implementation.

1. CheckRelatedItem (current behavior) - the main issue I see with this Projection is that the returned relatedItems field is a property that changes values based on the parameters passed in. This means that, as you imply, anytime you use the CheckRelatedItem projection, you must cache that URL separate from related URLs....which results in a non-RESTful behavior.
   • For example, suppose you need to call CheckRelatedItem three times for the same resource. This results in three entries in the cache, as each time you use this Projection you have to treat it as a separate resource entry.
     1. /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckRelatedItem&checkRelatedItem=8090c5aa-5c1a-460b-98c0-2bbfc13cb4b6
        • Returns "relatedItems": ["8090c5aa-5c1a-460b-98c0-2bbfc13cb4b6"],
     2. /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckRelatedItem&checkRelatedItem=8d60686a-cfad-42fb-88b0-33a482fa3346
        • Returns "relatedItems": ["8d60686a-cfad-42fb-88b0-33a482fa3346"],
     3. /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckRelatedItem&checkRelatedItem=ba2bbf71-4300-4f2f-8f65-b53e4bb6def4
        • Returns "relatedItems": ["ba2bbf71-4300-4f2f-8f65-b53e4bb6def4"],
   • So, as you noted, the way you have to work around this is by keeping all the params at all times, as those params make a difference in the cache. You cannot treat all three of these URLs as the same "HTTP Resource" because one of the properties is dependent on the params.
2. CheckRelatedItem (better behavior) - Better behavior here would be to ensure the property values do NOT change based on the parameters passed in. One example of doing this is the approach I suggested in Entities projections RestContract#165 (comment) . We'd refactor the return values so it's no longer a single property...instead it's set of properties which are shown/hidden based on the params. Something like: "relatedItems": { [uuid]: true/false}.
   • Now, let's look at the same three calls to CheckRelatedItem....same params, different return value:
     1. /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckRelatedItem&checkRelatedItem=8090c5aa-5c1a-460b-98c0-2bbfc13cb4b6
        • Returns "relatedItems": { "8090c5aa-5c1a-460b-98c0-2bbfc13cb4b6": true },
     2. /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckRelatedItem&checkRelatedItem=8d60686a-cfad-42fb-88b0-33a482fa3346
        • Returns "relatedItems": { "8d60686a-cfad-42fb-88b0-33a482fa3346": true },
     3. /server/api/core/items/a7e2ca60-5d0e-4d41-9457-9ab04702e18b?projection=CheckRelatedItem&checkRelatedItem=ba2bbf71-4300-4f2f-8f65-b53e4bb6def4
        • Returns "relatedItems": { "ba2bbf71-4300-4f2f-8f65-b53e4bb6def4": true },
   • The above may look like an extremely minor change...but, it's more RESTful behavior. Why? Because, now, all three of these calls has the potential to be cached in a single cache entry. Here's how it'd potentially work:
     1. After first call, cache (or really any client) "knows" we have an Item Resource that has one other related Item:

        "relatedItems": { 
             "8090c5aa-5c1a-460b-98c0-2bbfc13cb4b6": true 
        }, 
        

     2. After second call, cache (or really any client) discovers that same Item Resource has another related Item. Cached value changes to...

        "relatedItems": { 
            "8090c5aa-5c1a-460b-98c0-2bbfc13cb4b6": true,
            "8d60686a-cfad-42fb-88b0-33a482fa3346": true 
        }, 
        

     3. After third call, cache (or really any client) discovers that same Item Resource has another related Item. Cached value changes to...

        "relatedItems": { 
            "8090c5aa-5c1a-460b-98c0-2bbfc13cb4b6": true,
            "8d60686a-cfad-42fb-88b0-33a482fa3346": true,
            "ba2bbf71-4300-4f2f-8f65-b53e4bb6def4":  true
        }, 
        

   • As you can see, the client or caching mechanism can combine properties across these calls, because the value of any individual property is not dependent on parameters you've passed in. The only thing the parameters control is which property(ies) are returned in the "relatedItems" section.

The same sort of idea could also be applied to CheckSideItemInRelationship. It could be restructured so that the relatedItemLeft and relatedItemRight values no longer change based on the parameters passed in. So, it might for example, look more like this:

"relatedItemLeft": { 
    "a7e2ca60-5d0e-4d41-9457-9ab04702e18b": false,
},
"relatedItemRight": { 
    "a7e2ca60-5d0e-4d41-9457-9ab04702e18b": true,
},

To sum up, I don't think my suggestions will vastly modify this PR. But, these small changes would immediately make this endpoint behave in a more RESTful fashion, because the values of returned Item properties would no longer depend heavily on the parameters passed in. This allows caches or clients to more easily combine property information across multiple requests.

[abollini]

@tdonohue @benbosman sorry but I have to disagree about the direction of this PR. It is not about making it more or less restful we need to agree in advance on the REST contract picking the easier solution both for immediate implementation that for long term maintenance. As discussed in the REST contract DSpace/RestContract#165 (comment) I strongly feel that we can support our needs just using search methods without increase the complexity of our projection framework.

We should be very careful about having dynamic property in our rest resource this kind of stuff make much harder to support HATEOAS principles. The rest client should be able to get a comprensive description of how the response will look like, the ALPS standard that we would like to implement at some point would need to describe what a projection does and if we diverge from the spring data rest community practices.... we will be alone.
Another example of a violation of a common understanding due to the introduction of this projection is the "full" projection, the full will not include the dynamic properties so it will be not longer "full". Attempts to fix that would be complex and result in major performance overhead for 99% of use cases that really don't need these additional information.

[tdonohue]

Closing this implementation PR in favor of the approach defined in DSpace/RestContract#170 (a non-projection based approach). That said, if we find that approach falls short in significant ways, we can always revisit this idea to implement via a custom type of projection