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

Initial Pass at Lambda Filters - Seeking Feedback / Help #2781

Open
wants to merge 1 commit into
base: version-4
Choose a base branch
from

Conversation

bcameron1231
Copy link
Collaborator

This is the initial pass at building Lambda Expression for OData Filters.

Overall feelings - Not loving it, it feels a bit kludgy. It tried to implement the operations where there was a clear separation of AND/OR conditionals but that proved difficult and will need some thoughts on. Additionally, constructing without lambda using the FilterBuilder object, is also a bit kludgy and probably needs to be re-worked (as well as re-named).

I have another branch where I'm working on improving this, but haven't invested a ton yet to show. Would love to see if this effort can be improved.

  1. Allows for the ability pass in Standard Odata filter (for backwards compatibility).
  2. Allows for agility to use Lambda expressions with Intellisense
  3. Supports passing in date objects that can be automatically handled by the library
  4. Supports creating chained and/or conditionals
  5. Supports for re-using the underlying filter builder objects.

Sample usage

  //standard odata
  const r = await sp.web.lists.getByTitle("SPFx List").items.filter("Title eq 'Beau'")();    
  
  //lambda
  const r2 = await sp.web.lists.getByTitle("SPFx List").items.filter(i=> i.field("Title").equals("Beau").or(i=>i.field("SPFxCostCenter").equals("Information")))();
  const r3 = await sp.web.lists.getByTitle("SPFx List").items.filter(i=> i.field("Title").equals("Test").and(i=>i.field("SPFxCostCenter").equals("Information")))();
  const r4 = await sp.web.lists.getByTitle("SPFx List").items.filter(i=>(i.field("Title").equals("Beau")).or(i=>i.field("Title").equals("Cameron")))();
  
  //filter builder - very kludgy, needs to be re-worked
 let builder = new FilterBuilder();
 builder.field("Title").equals("Test");
 const r5 = await sp.web.lists.getByTitle("SPFx List").items.filter(builder.build())();

@juliemturner juliemturner mentioned this pull request Nov 20, 2023
@Tanddant
Copy link

Tanddant commented Nov 20, 2023

Hi @bcameron1231

I've been working on a similar concept as well - I went for less of a Lambda, and more of a CAMLjs style

OData.Where<ITask>()
    .NumberField("taskProductId").EqualTo(ProjectId)
    .And()
    .DateField("Created").LessThanOrEqualTo(new Date())
    .ToString();

Where The fields passed into DateField or NumberField (and so on) are required to be keys of the ITask interface

Output

taskProductId eq 58 and Created le '2023-11-20T14:18:45.455Z'

I've also implemented support for nested queries

I.e

OData.Where().Some([
    OData.Where<IProject>().NumberField("EditorId").EqualTo(60),
    OData.Where<IProject>().NumberField("EditorId").EqualTo(6),
    OData.Where<IProject>().NumberField("EditorId").EqualTo(85),
]).ToString()
( EditorId eq 60 or EditorId eq 6 or EditorId eq 85 )

These can also be nested (my main gripe with lambda expressions, is how unreadable they get at scale)

OData.Where().Some([
    OData.Where().All([
        OData.Where<any>().TextField("ProjectStatus").NotEqualTo("Done"),
        OData.Where<any>().DateField("Deadline").LessThan(new Date()),
    ]),
    OData.Where().All([
        OData.Where<any>().TextField("ProjectStatus").EqualTo("Critical"),
    ]),
]).ToString()
"( ( ProjectStatus neq 'Done' and Deadline lt '2023-11-20T14:29:02.608Z' ) or ( ProjectStatus eq 'Critical' ) )

@bcameron1231
Copy link
Collaborator Author

Thanks @Tanddant. Some great ideas here. Do you think you could create a branch with some of these changes implemented, or do you need some help with that?

We think there there is definitely a lot to like to about your implementation, but there may be a middle ground between the approach that I have and what you've implemented that could take this to the next level. I would love to see the inner workings of it.

Let me know how you'd like to best collaborate on this.

@Tanddant
Copy link

Hi @bcameron1231

Would love to help out - bit busy this week (need to finalize a bunch of stuff ahead of ESPC) - I spent 30 minutes trying to get the repo to work locally, but failed 😅

I've added my code at the end of the spqueryable.ts in my own fork, let me know if you have any questions - I'm at that phase of the solution where I'm like "I like this, it could work, and scale, but there's probably a reason no one has done this before if it's this easy, so I'm missing something obvious"

Also the naming could use a rework or two

Tanddant@4c43d58#diff-bdc963fcde94659fffe052eb963737c9440c80ed8435a039055a7892e3d8a7bb

@Tanddant
Copy link

There are also some filters I'm unaware of how works (never used) day, month, year, hour, minute, second, that maybe we should also think of a way to incorporate

docs

image

@Tanddant
Copy link

@bcameron1231 - I've moved my code here for now, as I keep iterating on it a bit, maybe we can have a chat post ESPC about what makes sense, I like the ideas you're working with using lambda expressions, but can certainly see the benefits of having type validation, maybe there's a golden middle ground

@bcameron1231
Copy link
Collaborator Author

@bcameron1231 - I've moved my code here for now, as I keep iterating on it a bit, maybe we can have a chat post ESPC about what makes sense, I like the ideas you're working with using lambda expressions, but can certainly see the benefits of having type validation, maybe there's a golden middle ground

That sounds good to me. I have some ideas we can start playing with. Let's connect in December.

@Tanddant
Copy link

Hey @bcameron1231, My calendar (and post-conference flu) is starting to clear up a bit!

Before I left i rewrote it to be based upon passing around a single class rather then just a string array (Rewrite branch), other then that I haven't had much time to look at it - I'm a bit stuck on getting PnPjs to run on my machine - so I can't really try it out in the full context, but let me know if there's anything you need me to do 😊

@bcameron1231
Copy link
Collaborator Author

@Tanddant Sent you a message on discord. We can try and get it resolved :)

@patrick-rodgers
Copy link
Member

@Tanddant - what's the latest status here? Do we want to merge this into v4 so other can contribute? Are you still actively developing? No pressure on timeline, just not sure where we are with this work. Thanks!

@Tanddant
Copy link

Tanddant commented Feb 8, 2024

@patrick-rodgers we have a well functioning solution, but for some odd reason I can't build it without commenting out some of it, and then "uncommenting" it while the code is running, @bcameron1231 wanted to take a look, but to my knowledge has been super busy 😊

@Tanddant
Copy link

Tanddant commented Apr 2, 2024

@bcameron1231 Have you had a chance to look at this? 😊

@bcameron1231
Copy link
Collaborator Author

@Tanddant Not yet. :( I've been extremely busy recently.

@Tanddant
Copy link

Tanddant commented Apr 2, 2024

@bcameron1231 All good, no worries - take care of yourself and relax when there's a bit of down time 😊

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.

None yet

4 participants