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

Turn PagedResourcesAssembler into an interface #2966

Open
reda-alaoui opened this issue Oct 26, 2023 · 4 comments
Open

Turn PagedResourcesAssembler into an interface #2966

reda-alaoui opened this issue Oct 26, 2023 · 4 comments
Labels
for: team-attention An issue we need to discuss as a team to make progress status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged

Comments

@reda-alaoui
Copy link
Contributor

reda-alaoui commented Oct 26, 2023

I'd like to decorate PagedResourcesAssembler. Could PagedResourcesAssembler be turned into an interface to allow this?
Note that I have the same kind of need with SlicedResourcesAssembler .

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 26, 2023
@gregturn
Copy link
Contributor

Could you elaborate on what you are wanting to do? When you say "make it an interface" suggests moving the existing code into default methods, allowing you to then override or wrap. Unfortunately, that's not possible because that class also has state, which is something you can't do with interfaces.

It's a public class meaning you can subclass it to and make changes. Or you can wrap it into some delegate pattern. Either way doesn't require changes to the framework.

So I'm really curious what your intentions are that can't already be achieved.

@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Oct 26, 2023

@gregturn I want to make RepresentationModelAssembler#toModel transactional while keeping using the current PagedResourcesAssemblerArgumentResolver.

Something like this:

class TransactionalPagedResourcesAssemblerArgument<T> implements PagedResourcesAssembler<T> {
   private final PagedResourcesAssembler delegate;

   @Override
   public <R extends RepresentationModel<?>> PagedModel<R> toModel(
      Page<T> page, RepresentationModelAssembler<T, R> assembler) {

    return delegate.toModel(page, entity -> transaction.execute(status -> assembler.toModel(entity)));
  }
}

Therefore when I say:

Could PagedResourcesAssembler be turned into an interface to allow this?

I mean something that could be as follow:

  • rename the current class to SimplePagedResourcesAssemblerArgumentResolver
  • push all its public methods signatures into a new interface named PagedResourcesAssemblerArgumentResolver
  • make SimplePagedResourcesAssemblerArgumentResolver implement PagedResourcesAssemblerArgumentResolver
  • reference PagedResourcesAssemblerArgumentResolver interface instead of the class as much as possible in spring-data-commons

@gregturn
Copy link
Contributor

Why put the transactional responsibility in an assembler?

I'm assuming this assembler is being used in the Spring MVC web controller. Why not let the web controller define the transactional boundaries?

The web controller would have the full context of how many DB operations are happening and thus should enclose that better.

The solution presented above opens you up to the risk that someone is going to use this operation and a couple others, all from the same web controller method, thus forcing more than one transaction to ensue.

From a pure software maintenance perspective, all the structural changes suggested are breaking and would require deprecation warnings and few release generations to achieve. We don't usually take on something like unless it's critical.

@gregturn gregturn added status: waiting-for-feedback We need additional information before we can continue for: team-attention An issue we need to discuss as a team to make progress labels Oct 26, 2023
@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Oct 26, 2023

The solution presented above opens you up to the risk that someone is going to use this operation and a couple others, all from the same web controller method, thus forcing more than one transaction to ensue.

Sorry, my snipped is not correct. Here is the corrected snippet:

class TransactionalPagedResourcesAssemblerArgument<T> implements PagedResourcesAssembler<T> {
   private final PagedResourcesAssembler delegate;

   @Override
   public <R extends RepresentationModel<?>> PagedModel<R> toModel(
      Page<T> page, RepresentationModelAssembler<T, R> assembler) {

    return readOnlyTransaction.execute(status -> delegate.toModel(page, assembler));
  }
}

Why put the transactional responsibility in an assembler?

Each call to the assembler represents a HATEOAS entity model links being built. It fetches data for permissions. We want to maintain the transaction open in this case to have 1 transaction per slice of data. 50 elements -> 50 calls to RepresentationModelAssembler to create links -> link creation is conditioned by database calls. Having 1 transaction keeps the EM level 1 cache open during the link creations. I want it to be systematic so that no developer forget to do that.

We don't usually take on something like unless it's critical.

I understand. But IMHO this kind of injectable components should be easy to decorate to allow framework users to customize their behaviour.

@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 Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for: team-attention An issue we need to discuss as a team to make progress status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

No branches or pull requests

3 participants