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

PUT and PATCH don't work, when custom entity lookup is configured. [DATAREST-1304] #1664

Closed
spring-projects-issues opened this issue Nov 8, 2018 · 5 comments
Assignees
Labels

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Nov 8, 2018

Hubert Wagener opened DATAREST-1304 and commented

Starting with version 3.0.0 save does not work correctly anymore , if a custom entity lookup strategy is configured. 

If we have an EntityLookupConfiguration like

@Configuration
public class RepositoryEntityLookupConfiguration extends RepositoryRestConfigurerAdapter {

    @Override
    public void configureRepositoryRestConfiguration(RepositoryRestConfiguration configuration) {
        configuration
                .withEntityLookup()
                .forRepository(SomeRepository.class, Some::getUid, SomeRepository::findByUid);
    }
}

assuming some Document like

@Document
public class Some {

    @Id
    String id;
    @NotBlank(message = "uid may not be null, empty or blank")
    @Indexed(unique = true)
    String uid;

    Programs programs;
    Bugs bugs;
}

then any update operation fails with 409.

Actually, when the frameworks tries to save (updates the persistent data), the id of the persistence layer (id in our case) is substituted by the lookup key (uid), and then the save operation inserts a new document with a uid forbidden by the unique id.

The "bug" is in PersistentEntityResourceHandlerMethodArgumentResolver#resolveArgument (lines 139-150), where the persistence-id-property is set.

 

     // here it is checked whether we have an update and the persistence-id-property is set to entity-id
            PersistentEntity<?, ?> entity = resourceInformation.getPersistentEntity();
            boolean forUpdate = objectToUpdate.isPresent();
            Optional<Object> entityIdentifier = objectToUpdate.map(it -> entity.getIdentifierAccessor(it).getIdentifier());

            entityIdentifier.ifPresent(it -> entity.getPropertyAccessor(obj).setProperty(entity.getRequiredIdProperty(),
                    entityIdentifier.orElse(null)));

            // unfortunately, here the correct value is overwritten (possibly with a wrong value, as in case custom lookup configuration)
            id.ifPresent(it -> {
                ConvertingPropertyAccessor accessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(obj),
                        conversionService);
                accessor.setProperty(entity.getRequiredIdProperty(), it);
            });

I'm not quite sure under with conditions the requiredIdProperty should be set;
perhaps only if (!forUpdate) or (!forUpdate && !noCustomLookupConfig) or something similar along the lines.

The result then may be something like:

if(!forUpdate) {
            id.ifPresent(it -> {
                ConvertingPropertyAccessor accessor = new ConvertingPropertyAccessor(entity.getPropertyAccessor(obj),
                        conversionService);
                accessor.setProperty(entity.getRequiredIdProperty(), it);
            });
        }

This is actually equivalent to what was implemented in 2.x.x branches. 

Note this does still not give a valid solution of the problem in the sense of the requirements in DATA-REST-1050 in case of EntityLookupConfiguration. This is because in case of creating a new resource, a lookup by the same custom key does not find the created resource in general.

But at least, it allows us to upgrade existing applications to Spring Boot 2.0, since we are not relying on an idempotent PUT anyway. With the current version, we see no way to upgrade to Spring Boot 2 at all


Affects: 3.0.11 (Kay SR11)

Issue Links:

  • DATAREST-1050 Deserialization of request body for creating PUT requests has to consider identifier derived from the URI
    ("depends on")

  • DATAREST-1224 Spring Data REST with Custom Resource URI Fails on PUT/PATCH
    ("is duplicated by")

  • DATAREST-1433 UnsatisfiedDependencyException with custom ID mapping in Kotlin

  • DATAREST-1405 Using RepositoryRestConfigurer withEntityLookup does not work for a PUT on existing records

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 13, 2018

Oliver Drotbohm commented

I have a few tweaks ready to improve on this but I am puzzle by the last section of your summary. Even if we change the implementation to what you suggest, we'd still end up with the lookup key (your getUid()) being attempted to be set as the actual entity id, which is wrong, isn't it?

I think as long as we don't have means in place to set the correct property on the entity, we're going to have to disable entity creation via PUT completely. Thoughts?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Dec 13, 2018

Hubert Wagener commented

Yes, you're correct.

The 2.x.x solution (conditional on !forUpdate) would work, if and only if PUT is used only for updates, and never for inserts in case of custom lookup keys. The client has to be aware of this. This would be the minimal requirement for us to use version 3.x.x (This is what version 2.x.x provided). We would be happy with this solution as a first step anyway.

A more reliable solution is probably to actively refuse inserts, when custom lookup is configured ( i.e. no upsert attempt as soon as custom lookup is configured for the entity class). To put it by other words: conditional on (!forUpdate) or (!forUpdate && !noCustomLookupConfig()) [Whatever may be an implementation of noCustomLookupConfig]

For a full solution, additional information for the entity in the LookupRegistrar is required. The identifierMapping retrieves the ID. What is needed additionally now is an "businessIdentityToEntityMapping", that fills the entity properties in the "right" places. I.e. such that the lookup on the given identifier retrieves exactly this entity.

In our example case above, that could simply something based on be Some#setUid(ID identity). In case of composite keys several entity attributes might be affected.

This kind of information can not retrieved from the identifierMapping and Lookup-Information though, i guess, but has to be provided by the programmer. This in turn breaks the existing API for providing "businessIdentityToEntityMapping" functions, ....

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Feb 12, 2019

sante85 commented

i edit this in spring data jpa test

and resul test is in error

 

@Test // DATAREST-523public void augmentsOneToManyCollectionAssociationUsingPost() throws Exception {

List<Link> links = preparePersonOrderResources(new Person("Frodo", "Baggins"),
new Order(), new Order());

Link frodosSiblingsLink = links.get(0).expand();
Link bilboLink = links.get(1);

for (int i = 1; i <= 2; i++) {

mvc.perform(post(frodosSiblingsLink.getHref()).// content(bilboLink.getHref()).// contentType(TEXT_URI_LIST)).// andExpect(status().isNoContent());

mvc.perform(get(frodosSiblingsLink.getHref())).// andExpect(jsonPath("$._embedded.people", hasSize(i)));
}
}

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 7, 2019

sante85 commented

Good morning,

I not use custom entity lookup;

this is my configuration

 

@Configuration
public class RepositoryConfiguration implements RepositoryRestConfigurer {

@Override
public void configureRepositoryRestConfiguration(RepositoryRestConfiguration config) {

config.exposeIdsFor(Log.class);
config.exposeIdsFor(MessagePacket.class);
config.exposeIdsFor(RailwayStation.class);
config.exposeIdsFor(Session.class);

config.setReturnBodyOnCreate(true);
config.setReturnBodyOnUpdate(true);
}

@Override
public void configureConversionService(ConfigurableConversionService service) {
service.addConverter(stringURIToLongConverter());
}

@Bean
public Converter<String, Long> stringURIToLongConverter() {
return new StringToLongConverter();
}
}

public class StringToLongConverter implements Converter<String, Long> {
@Override
public Long convert(String source) {
int lastSlash = source.lastIndexOf('/');
if (lastSlash != -1)
source = source.substring(lastSlash + 1);
return Long.parseLong(source);
}
}

 

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 7, 2019

Hubert Wagener commented

Thanks, looks nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants