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.

Sign up for GitHub

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

Jump to bottom

feat(doctrine): searchfilter using id instead of full IRI when it's not an int #5771

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat(doctrine): searchfilter using id instead of full IRI when it's not an int #5771

wants to merge 1 commit into from

Conversation

mrossard
Copy link
Contributor

@mrossard mrossard commented Aug 23, 2023 •
edited

Q A
Branch? main

This brings support for searches likes /entities/?subEntity=subEntityId when subEntity id is not int $id.
This is a follow up on #5760

@mrossard mrossard force-pushed the search-on-nested-id branch 2 times, most recently from 7d50d3f to 6cd4adf Compare August 23, 2023 15:56
@mrossard mrossard mentioned this pull request Aug 23, 2023
Closed
soyuka
soyuka reviewed Aug 24, 2023
View reviewed changes
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

A new feature should target main

features/doctrine/search_filter.feature Outdated
@@ -1048,7 +1048,7 @@ Feature: Search filter on collections
Scenario: Filters can use UUIDs
Given there is a group object with uuid "61817181-0ecc-42fb-a6e7-d97f2ddcb344" and 2 users
And there is a group object with uuid "32510d53-f737-4e70-8d9d-58e292c871f8" and 1 users
When I send a "GET" request to "/issue5735/issue5735_users?groups[]=/issue5735/groups/61817181-0ecc-42fb-a6e7-d97f2ddcb344&groups[]=/issue5735/groups/32510d53-f737-4e70-8d9d-58e292c871f8"
When I send a "GET" request to "/issue5735/issue5735_users?groups[]=61817181-0ecc-42fb-a6e7-d97f2ddcb344&groups[]=/issue5735/groups/32510d53-f737-4e70-8d9d-58e292c871f8"
Copy link
Member

Choose a reason for hiding this comment

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

don't change a test please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry, i was doing that at the same time as #5760 , i'll make separate tests.

src/Doctrine/Orm/Filter/SearchFilter.php
parent::__construct($managerRegistry, $logger, $properties, $nameConverter);

$this->iriConverter = $iriConverter;
$this->identifiersExtractor = $identifiersExtractor;
$this->propertyAccessor = $propertyAccessor ?: PropertyAccess::createPropertyAccessor();
$this->resourceMetadataFactory = $resourceMetadataFactory;
$this->resourceClassResolver = $resourceClassResolver;
Copy link
Member

Choose a reason for hiding this comment

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

lots of new dependencies I wish we wouldn't have...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that's mostly why i didn't include this part in the previous PR. And that's kind of the point I was making in #5687 - some of the metadata is not easy to get to at times.

src/Doctrine/Orm/Filter/SearchFilter.php Outdated
// dig into the subResource metadata to find out which field we're looking at
if ($this->isResourceClass($associationResourceClass) && $this->resourceMetadataFactory) {
$associationApiMetadata = $this->resourceMetadataFactory->create($associationResourceClass);
$variables = $associationApiMetadata->getIterator()->current()->getUriVariables();
Copy link
Member

Choose a reason for hiding this comment

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

you should call getOperation instead of getIterator()->current()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll check that, thanks.

src/Doctrine/Orm/Filter/SearchFilter.php
$variables = $associationApiMetadata->getIterator()->current()->getUriVariables();
if (1 === \count($variables)) { // otherwise let's just give up at this point, too complicated
$varName = array_key_first($variables);
$associationResourceApiIdField = $variables[$varName]->getIdentifiers()[0]; // this will be needed to get the correct value
Copy link
Member

Choose a reason for hiding this comment

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

agreed this is too complicated and deserves its own search filter...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with multiple identifiers this becomes even clumsier to handle, and i'm not even sure what the request should look like anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Best would be to make some mix with #5732 to handle complicated stuff not sure how it'd apply on filters though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A similar handlelinks option in filters?

That's a really interesting idea, but every filter would need to be reworked, that's quite an impact. I'd keep that idea in mind with #2400 for a major overhaul of filters, maybe?

@mrossard mrossard changed the base branch from 3.1 to main August 24, 2023 11:29
@mrossard mrossard changed the base branch from main to 3.1 August 24, 2023 11:37
@mrossard
Copy link
Contributor Author

mrossard commented Aug 24, 2023 •
edited

A new feature should target main

Since this is based on a commit from 3.1 I wasn't sure this was the correct move...I'd have to rebase and make sure it still works, which feels awkward...

@mrossard mrossard changed the base branch from 3.1 to main August 25, 2023 10:43
@mrossard mrossard marked this pull request as draft August 26, 2023 11:55
@mrossard mrossard marked this pull request as ready for review September 2, 2023 08:13
@mrossard
Copy link
Contributor Author

mrossard commented Sep 2, 2023

Finally found some time to rebase onto the updated main...would it be ok this way?

@mrossard mrossard requested a review from soyuka September 2, 2023 08:14
@mrossard mrossard mentioned this pull request Sep 4, 2023
Open
@stale Stale
Copy link

stale bot commented Nov 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Nov 2, 2023
@mrossard
Copy link
Contributor Author

mrossard commented Nov 2, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

just stopping this from automatically closing...no time to work on it right now but i still want to...

@stale stale bot removed the stale label Nov 2, 2023
@soyuka
Copy link
Member

soyuka commented Nov 2, 2023

my bad :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soyuka soyuka Awaiting requested review from soyuka

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mrossard @soyuka