Skip to content

Added nested conditional filter support#41

Merged
jongpie merged 1 commit intofeature/new-unlocked-packagesfrom
nested-query-filters
May 31, 2023
Merged

Added nested conditional filter support#41
jongpie merged 1 commit intofeature/new-unlocked-packagesfrom
nested-query-filters

Conversation

@jamessimone
Copy link
Collaborator

No description provided.

@jamessimone jamessimone requested a review from jongpie May 24, 2023 19:11
@jongpie
Copy link
Owner

jongpie commented May 31, 2023

@jamessimone I just reviewed this again, and I love it! I think I'm good with the class & enum names - I'll go ahead and merge this into my branch for PR #39, and then I'll release the new packages this week.

Thanks again for adding this!

@jamessimone
Copy link
Collaborator Author

Awesome, sounds good!

@jongpie jongpie merged commit 82f344c into feature/new-unlocked-packages May 31, 2023
@jongpie jongpie deleted the nested-query-filters branch May 31, 2023 18:48
@jongpie
Copy link
Owner

jongpie commented Jun 27, 2023

@jamessimone I'm going to revert this PR for the moment - it looks like there wasn't an assert in the new test method, so I added System.assertEquals(expected, actual.getQuery()); (plus a small change to include the Name field in the expected query, since Query auto-adds the name field.... apparently), and the generated where clause is a bit jumbled.

  • Expected: `SELECT Id, Name FROM Contact WHERE LastName = :bindVar0 OR LastName = :bindVar1 OR (FirstName = :bindVar2 AND (LastName != :bindVar3 OR LastName != :bindVar4))
  • Actual: SELECT Id, Name FROM Contact WHERE (AND (FirstName = ':bindVar2' OR (LastName != ':bindVar3' LastName != ':bindVar4'))) AND (LastName = ':bindVar1') AND

In the 'actual' value, there are some extra AND strings being added, and more notably, the ordering of the filter string is different. I think this is all related to some existing logic in the SOQL class that does some sorting on the list of QueryFilter when generating the query string via Query.getQuery(). It's been so long since I worked on some of this code, I don't remember why exactly it's doing the sorting, but I know it was a purposeful decision that probably made sense when it was added 5 years ago😅 ), and I don't want to risk breaking existing behavior by changing it right now. I'm planning to do a big refactor of the entire codebase over the coming weeks; I'll re-merge your changes once I've dived a little deeper into the code & done some cleanup.

jongpie added a commit that referenced this pull request Jun 27, 2023
@jamessimone
Copy link
Collaborator Author

Sure, makes sense!

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.

2 participants