Added support for database access levels and new query methods with binds #49
Conversation
superkensington
commented
May 7, 2024
superkensington
commented
- Resolves Use the new method Database.queryWithBinds #35 and Add support for 'WITH USER_MODE' #36
- Also updated API versions to 60.0
|
@superkensington thanks so much for working on this & apologies for not responding sooner! I unfortunately have limited time over the next few weeks to work on open source stuff, but I will review this PR ASAP. As soon as I have a chance to thoroughly review this, I'll let ya know what feedback I have. |
There was a problem hiding this comment.
@superkensington I finally had a chance to go through this in depth, and this looks so good! It looks like you've added several enhancements + improved several things + kept things backwards-compatible for existing functionality. I really appreciate all the work you've done, this is a stellar PR.
I've added a few comments to the PR, but there's not really much to change
- A few comments about standardizing a variable name
- We could go ahead and switch to API v61.0 since Summer '24 is now live
- A comment or two about the idea of auto-generating bind variable names. This doesn't have to be implemented as part of this PR, but I'd love to at least get your thoughts on the overall idea.
Let me know what you think! I'm also happy to help make some of the changes if you'd like - whatever works best for you.
Thanks again!
| @@ -64,6 +64,11 @@ global class RecordSearch extends SOSL { | |||
| return this.setHasChanged(); | |||
| } | |||
|
|
|||
| global RecordSearch withAccessLevel(System.AccessLevel accessLevel) { | |||
| this.accessLevel = accessLevel; | |||
There was a problem hiding this comment.
I think this should call the super class's method right? Query and AggregateQuery are both using the super method, like this:
| this.accessLevel = accessLevel; | |
| super.doWithAccessLevel(accessLevel); |
| aggregateFunction.name(), | ||
| queryField.toString(), | ||
| SOQL.getOperatorValue(operator), | ||
| (String.isNotBlank(bindWithKey) ? ':' + bindWithKey : new QueryArgument(value).toString()) |
There was a problem hiding this comment.
I love how you've kept it backwards compatible with QueryArgument, this is great!
| return this.havingAggregate(aggregateFunction, queryField, operator, value, null); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this new method overload signature makes sense, especially for trying to keep things backwards compatible with QueryArgument. But my small complaint is:
- I want to keep the backwards-compatability you've done for using
QueryArgument - I plan to always use the query with binds functionality (personal preference + my impression is that it's more performant)
- I don't want to have to specify a bind variable name every time, I'd prefer the bind variable name be auto-generated
So I'm thinking there could be another method to enable automatically generating bind variable names. Something like this:
private Boolean generateBindVariables = false;
private Integer generatedBindVariableCounter = 0;
global AggregateQuery withBinds() {
this.generateBindVariables = true;
}And then in this havingAggregate() overload, auto-generate the bind var name when enabled:
| return this.havingAggregate(aggregateFunction, queryField, operator, value, null); | |
| } | |
| String bindVariableName = this.generateBindVariables == false ? null : 'bindVar' + this.generatedBindVariableCounter++; | |
| return this.havingAggregate(aggregateFunction, queryField, operator, value, bindVariableName); | |
| } |
Example usage would look something like this:
AggregateQuery aggregateQuery = new AggregateQuery(Schema.Account.SObjectType)
.withBinds()
.groupByField(Schema.Account.Name)
.addAggregate(SOQL.Aggregate.COUNT, Schema.Account.Id)
.havingAggregate(SOQL.Aggregate.COUNT, Schema.Account.Id, SOQL.Operator.GREATER_THAN, someVariable);I think the same idea would apply in the Query class too. Let me know what you think.
| global AggregateQuery withAccessLevel(System.AccessLevel mode) { | ||
| super.doWithAccessLevel(mode); |
There was a problem hiding this comment.
For consistency, I think the variable should be called accessLevel instead of mode
| global AggregateQuery withAccessLevel(System.AccessLevel mode) { | |
| super.doWithAccessLevel(mode); | |
| global AggregateQuery withAccessLevel(System.AccessLevel accessLevel) { | |
| super.doWithAccessLevel(accessLevel); |
| global Query withAccessLevel(System.AccessLevel mode) { | ||
| super.doWithAccessLevel(mode); |
There was a problem hiding this comment.
Same thing here - for consistency, I think the variable should be called accessLevel instead of mode
| global Query withAccessLevel(System.AccessLevel mode) { | |
| super.doWithAccessLevel(mode); | |
| global Query withAccessLevel(System.AccessLevel accessLevel) { | |
| super.doWithAccessLevel(accessLevel); |
| @@ -1,5 +1,5 @@ | |||
| <?xml version="1.0" encoding="UTF-8" ?> | |||
| <ApexClass xmlns="http://soap.sforce.com/2006/04/metadata"> | |||
| <apiVersion>58.0</apiVersion> | |||
| <apiVersion>60.0</apiVersion> | |||
There was a problem hiding this comment.
Since Summer '24 release is now live in all orgs, we could probably bump this to 61.0 now.
|
@jongpie the noted items have been cleaned up. I like your idea of auto-generating the variable names! I've done some initial work to implement this, but there's a small issue that will require some changes to the way that filters are handled. Since they are currently stringified as they are added to a query, if you don't invoke |
|
@superkensington thanks for working on those changes! I'm reviewing & testing your changes today in PR #50 - since that PR has all of the changes from this PR (plus other changes), I'll close this one out, and will add any feedback to PR #50. My hope is to finally release your changes sometime this week 🥳 |
