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

feat(graph): add advanced query behaviour #3074

Merged
1 commit merged into from
Jul 15, 2024

Conversation

JakeStanger
Copy link
Contributor

@JakeStanger JakeStanger commented Jun 20, 2024

Category

  • Bug fix?
  • 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.

@patrick-rodgers
Copy link
Member

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?

@JakeStanger
Copy link
Contributor Author

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.

@juliemturner
Copy link
Collaborator

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.

@patrick-rodgers
Copy link
Member

Yep, thanks for explaining

@JakeStanger
Copy link
Contributor Author

JakeStanger commented Jul 4, 2024

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

@juliemturner juliemturner closed this pull request by merging all changes into pnp:version-4 in b1dd32b Jul 15, 2024
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.

3 participants