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

Allow customization of LinkCollector #2042

Closed
alienisty opened this issue Jul 14, 2021 · 10 comments
Closed

Allow customization of LinkCollector #2042

alienisty opened this issue Jul 14, 2021 · 10 comments
Assignees
Labels
status: feedback-provided Feedback has been provided status: pending-design-work Needs design work before any code can be developed

Comments

@alienisty
Copy link

Spring Data Rest creates links in a resource for all its associations which types have an exported REST repository, otherwise they are inlined as their configured JSON representation.
The created links are represented in terms of the containing resource though so the same referenced resource could have mutliple identifiers, which is not cache friendly.
For example given the following entities:

@Entity
public class Person {
  @Id
  private Long id;
  private String name;
  private String surname;
  private int age;
  @OneToOne(cascade = ALL)
  private Address address;
        ...
}

@Entity
public class Address {
  @Id
  private Long id;
  private String address;
  private Sting zip;
   ...
}

If both entities have an exported REST repository, a JSON representation would look something like:

{
  "name" : "name",
  "surname" : "surname",
  "age" : 11,
  "_links" : {
    "self" : {
      "href" : "http://localhost/people/1"
    },
    "person" : {
      "href" : "http://localhost/people/1"
    },
    "address" : {
      "href" : "http://localhost/people/1/address"
    }
  }
}

If the related address has id=2, for example, then the same instance can be addressed as both:

http://localhost/people/1/address
and
http://localhost/addresses/2

We believe that a more cache friendly representation would be as:

{
  "name" : "name",
  "surname" : "surname",
  "age" : 11,
  "_links" : {
    "self" : {
      "href" : "http://localhost/people/1"
    },
    "person" : {
      "href" : "http://localhost/people/1"
    },
    "address" : {
      "href" : "http://localhost/addresses/2"
    }
  }
}

Expanding on that, if the Person entity had an association to a collection of Addresses, we also believe that links to that collection could be more cache friendly if represented as:

{
  "name" : "name",
  "surname" : "surname",
  "age" : 11,
  "_links" : {
    "self" : {
      "href" : "http://localhost/people/1"
    },
    "person" : {
      "href" : "http://localhost/people/1"
    },
    "addresses" : [{
      "href" : "http://localhost/addresses/1"
    },{
      "href" : "http://localhost/addresses/2"
    }]
  }
}

We use that representation in our application and we were able to do that by overriding the LinkCollector bean.

Recently though, as reported in #1981, RepositoryRestMvcConfiguration can no longer be subclassed, so we would like to either restore the previous support, or add the ability to obtain the desired behaviour.

@odrotbohm
Copy link
Member

Thanks for writing this up.

If the related address has id=2, for example, then the same instance can be addressed as both:

http://localhost/people/1/address
and
http://localhost/addresses/2

That's not quite correct. These two URI identify semantically different resources. The latter is the actually linked address, while the former is an association resource. I.e. it represents the assignment between the two aggregates and allows the modification of that assignment in idempotent fashion. It allows using PUT requests to idempotently update the association without having to also submit – and thus also update – all other fields of the resource, which is something the suggested link format doesn't actually support at all.

Can you elaborate what you mean with "cache friendly"? The association resources' responses can be cached as well. If you'd like to avoid the additional request to actually access the representations of the related resources, they can be inlined in the person's representation by providing excerpt projections.

Another aspects that's a little odd with the suggested approach is that you'd have to move the addresses out of the _links object into the root document to produce a representation for a proper update. That seems less straightforward than treating associations separately through a dedicated resource.

All that said, I'm a bit surprised that you allegedly got these customizations working by only exchanging the LinkCollector bean. Let's say we turned that into an interface and introduced a callback on RepositoryRestConfigurer to customize the default instance. Would that get you back on track? Are there any other components you need to customize to make this particular use case work?

@odrotbohm odrotbohm added status: pending-design-work Needs design work before any code can be developed status: waiting-for-feedback We need additional information before we can continue and removed status: waiting-for-triage An issue we've not yet triaged labels Jul 14, 2021
@odrotbohm odrotbohm self-assigned this Jul 14, 2021
@alienisty
Copy link
Author

Thanks for your response. I should have specified better in my description that we are using custom caching in our application, not just using the browser's cache.
As I understand it, the represenation provided by Spring Data Rest is an indirect address to the actual resource currently linked to the aggregate, so, for example, "http://localhost/people/1/address" never changes, but the actual referenced resource may change, which is nice in a sense, but it doesn't work for our caching system.

All that said, I'm a bit surprised that you allegedly got these customizations working by only exchanging the LinkCollector bean. Let's say we turned that into an interface and introduced a callback on RepositoryRestConfigurer to customize the default instance. Would that get you back on track? Are there any other components you need to customize to make this particular use case work?

All we do is to extend LinkCollector and override getLinksFor() and getLinksForNested() and the latter is only needed to support a legacy system which we are slowly decommissioning. So, if you keep the default link collector implementation open for extension and you add your proposed interface and callback, that will work for us.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jul 15, 2021
@odrotbohm
Copy link
Member

What does the upgrade situation you're in look like? I guess you're on a SD REST version that still allows sub-classing of RRMC and would have to touch the code anyway, right? I wonder how rapid we can actually move to LinkCollector as interface to prevent hassle in projects currently customizing it. But it feels like there's going to be a need to touch the code anyway, if you have extended the class in the first place.

@odrotbohm odrotbohm changed the title Standard associations links format is not cache friendly Allow customization of LinkCollector Jul 15, 2021
@alienisty
Copy link
Author

We are on latest 3.5.2, but we monkey-patched DelegatingHandlerMapping to make it public so that subclassing does not fail at runtime.
Adapting our code should be pretty simple and doing that so that we can use a supported customisation mechanism will be lots better than having our code to break on each upgrade.

@odrotbohm
Copy link
Member

There's a build running containing the suggested changes. We have a release scheduled for tomorrow. Would be cool if you had a chance to check whether the snapshots allow you to plug what you need. RepositoryRestConfigurer has ….customize(…) methods now for component post-processing.

@alienisty
Copy link
Author

I surely can, whould that be 3.6.0-SNAPSHOT? Is there a snapshot for spring-data-bom that would contain it? We get SPDR through spring-boot-dependencies and it would be easier to override that version for testing.

@odrotbohm
Copy link
Member

Setting <spring-data-bom.version>2021.1.0-SNAPSHOT</spring-data-bom.version> should give you the latest Spring Data snapshots overall. Be sure to also set <spring-hateoas.version>1.4.0-SNAPSHOT</spring-hateoas.version> as that gets pulled in separately and IIRC we need a coordinated update for this release.

@alienisty
Copy link
Author

Moving to the new customization mechanism has been straightforward and it works as expected.

@odrotbohm
Copy link
Member

Lovely, happy coding! 🙌

@flaviut
Copy link

flaviut commented Jan 29, 2024

Here is a starting point for anyone interested in having the links array contain canonical URLs rather than "association resources". It works very well for me, but is likely probably a completely wrong way to do everything.

fun Links.replace(link: Link): Links {
    val linkMap = this.associateBy { it.rel }.toMutableMap()
    linkMap[link.rel] = link
    return Links.of(linkMap.values)
}

class AbsoluteLinkCollector(oldCollector: LinkCollector) : LinkCollector {
    val logger = createLogger()

    private val entities = oldCollector.javaClass.getDeclaredField("entities")
        .apply { isAccessible = true }
        .get(oldCollector) as PersistentEntities
    private val associationLinks = oldCollector.javaClass.getDeclaredField("associationLinks")
        .apply { isAccessible = true }
        .get(oldCollector) as Associations
    private val links = oldCollector.javaClass.getDeclaredField("links")
        .apply { isAccessible = true }
        .get(oldCollector) as SelfLinkProvider

    override fun getLinksFor(obj: Any) = this.getLinksFor(obj, Links.NONE)

    override fun getLinksFor(obj: Any, existing: Links): Links {
        var newLinks = existing.replace(links.createSelfLinkFor(obj).withSelfRel())

        val entity = entities.getRequiredPersistentEntity(obj::class.java)
        entity.doWithAssociations { assoc ->
            try {
                if (!associationLinks.isLinkableAssociation(assoc)) {
                    return@doWithAssociations
                }

                val ownerMetadata = associationLinks.getMetadataFor(assoc.inverse.owner.type)
                val propertyMapping = ownerMetadata.getMappingFor(assoc.inverse)
                val rel = propertyMapping.rel

                // if it is a collection, we can't get the value
                if (assoc.inverse.isCollectionLike) {
                    return@doWithAssociations
                }

                val fieldValue = obj::class.java.getDeclaredField(assoc.inverse.name)
                    .apply { isAccessible = true }
                    .get(obj) ?: return@doWithAssociations

                newLinks = newLinks.replace(links.createSelfLinkFor(fieldValue).withRel(rel))
            } catch (e: Exception) {
                // without this, we get a useless and confusing "Failed to write request"
                // error response.
                logger.error("Error getting links for $obj", e)
                throw e
            }
        }

        return newLinks
    }

    override fun getLinksForNested(obj: Any, existing: Links): Links {
        return this.getLinksFor(obj, existing)
    }
}


@Configuration
class RestConfiguration : RepositoryRestConfigurer {
    override fun customizeLinkCollector(collector: LinkCollector): LinkCollector {
        return AbsoluteLinkCollector(collector)
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided status: pending-design-work Needs design work before any code can be developed
Projects
None yet
Development

No branches or pull requests

4 participants