Skip to content

Conversation

@devmaslov
Copy link

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

I enabled elasticsearch support for one entity and everything worked fine until I made a put request. New entity was about to be created instead of updating existing one.

It appears that when elasticsearch support is turned on even on put and delete requests entities are loaded from elastic and denormalized from the payload - as a result doctrine see them as detached entities. This leads to a new entity create on put operation and 500 error on delete.

I think elasticsearch item data provider shouldn't support either put or delete operations. Or is there a better way to fix this issue?

@devmaslov devmaslov changed the title Disabled put and delete operations support for elastic item data provider Disable put and delete operations support for elastic item data provider Sep 16, 2019
@dunglas
Copy link
Member

dunglas commented Sep 17, 2019

PATCH should also be disabled

*/
public function supports(string $resourceClass, ?string $operationName = null, array $context = []): bool
{
if (\in_array($operationName, ['put', 'delete'], true)) {
Copy link
Member

@soyuka soyuka Sep 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we can relate on the operation name here as the route method can be different from the operation name no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point. I can just rename the operation to "put2" via annotations for example and the fix won't work already.

Option #2
Use item operation "method" attribute in the resource metadata. It's a bit better, but we can't rely on it either, since it's possible to omit it. You can just set "route_name" and there won't be "method" property.

Option #3
Another option would be to directly check current request method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh yeah you're right about option 2... @teohhanhui what would you do?

@devmaslov
Copy link
Author

PATCH should also be disabled

Agree. Then probably it makes sense to support only "GET" operations, not just exclude "put", "patch", "delete".

@teohhanhui
Copy link
Contributor

teohhanhui commented Sep 19, 2019

Can we just make sure that no default PUT, DELETE, and PATCH operations are generated when elasticsearch support is enabled? I don't think there is a general fix that wouldn't result in false positives with custom operations.

@devmaslov
Copy link
Author

I guess our best shot then to just check whether it's a PUT, DELETE or PATCH operation based on operation method. If it's a custom operation without "method" specified for it, elasticsearch support should be disabled via attributes directly, if it's necessary.

Updated the pull request.

@soyuka soyuka requested a review from teohhanhui September 20, 2019 07:26
@devmaslov devmaslov closed this by deleting the head repository Aug 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants