pnpjs icon indicating copy to clipboard operation
pnpjs copied to clipboard

feat(graph): add advanced query behaviour

Open JakeStanger opened this issue 1 year ago • 5 comments

Category

  • [ ] Bug fix?
  • [x] New feature?
  • [ ] New sample?
  • [ ] Documentation update?

What's in this Pull Request?

Hey, been a while!

Currently enabling advanced query capabilities is supported, but a two-step process:

const filter = "companyName ne null and NOT(companyName eq 'Microsoft')";
const query = graph.users.using(ConsistencyLevel()).filter(filter);
query.query.set("$count", "true");
const res = await query();

This PR adds a new behaviour which simply combines the two steps to make this more ergonomic:

const filter = "companyName ne null and NOT(companyName eq 'Microsoft')";
const res = await graph.users.using(AdvancedQuery()).filter(filter)();

I have updated docs but have NOT updated the relevant tests yet. If you can let me know whether you'd be happy to accept this change, I'll go through and update the tests.

JakeStanger avatar Jun 20 '24 10:06 JakeStanger

Hi Jake! Looks like good work to me :). The only thing I am not sure of is the name - would something like IncludeCount or WithCount or something else be more clear?

patrick-rodgers avatar Jun 27 '24 13:06 patrick-rodgers

Cheers, I'll try and update the tests for you in the next couple of days.

The only thing I am not sure of is the name - would something like IncludeCount or WithCount or something else be more clear?

My reasoning for the name is this performs the two 'setup actions' necessary for what MS refer to as 'advanced query capabilities'. I've just realised I helpfully left off the link to that in the original post: https://learn.microsoft.com/en-us/graph/aad-advanced-queries?tabs=http.

I'd say that naming it specifically after the count would not be correct, as it's doing more than that (you can enable count without the consistency level header).

If you disagree still, I'll change it and add a sentence or two explaining that the behaviour is useful for enabling advanced queries in the docs.

JakeStanger avatar Jun 27 '24 15:06 JakeStanger

Yep, now that we understand your thoughts about the naming that makes sense. As far as testing we'd like to see a unit test specific to the feature, but wouldn't think we'd adjust existing tests, but I'm not 100% sure I have enough details so just wanted to clarify.

juliemturner avatar Jul 01 '24 16:07 juliemturner

Yep, thanks for explaining

patrick-rodgers avatar Jul 03 '24 17:07 patrick-rodgers

Cheers both. I've added what are effectively copies of the existing advanced query tests, using the new behaviour for these.

JakeStanger avatar Jul 04 '24 09:07 JakeStanger