-
Notifications
You must be signed in to change notification settings - Fork 1.5k
JAVA-2715: Query builder for new lookup stage #434
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
Conversation
You’re welcome! 😉 |
Fixed CS issues
*/ | ||
public class Variable<TExpression> { | ||
private final String name; | ||
private TExpression value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just would like to use a few more tests of the first overload, as well as the javadoc fixes.
* @since 3.7 | ||
* | ||
*/ | ||
public static Bson lookup(final String from, final List<? extends Bson> pipeline, final String as) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a unit and functional test for this overload. Currently it's untested, but I want to make sure we catch any issues when let
is null.
} | ||
|
||
/** | ||
* Creates a $lookup pipeline stage for the specified filter |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's fix this comment now, as there is no filter
, but rather a pipeline. Perhaps:
Creates a $lookup pipeline stage, joining the current collection with the one specified in from
using the given pipeline
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I realize the comment in the existing lookup method is misleading as well. Feel free to fix that one too if you're so inclined.)
* Creates a $lookup pipeline stage for the specified filter | ||
* | ||
* @param from the name of the collection in the same database to perform the join with. | ||
* @param let specifies variables to use in the pipeline field stages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace "specifies" with "the".
) | ||
|
||
collection.insertDocuments( | ||
Document.parse('''{ "_id" : 1, "student" : "Ann Aardvark", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build failure is due to trailing whitespace on this line. If you run `./gradlew driver-core:check' it should fail for you too.
Thanks for the work on this @fhassak! It's now merged to master. |
JAVA-2715: Support lookup stage in Aggregates builder
thanks to @csarrazi who helped me for this contribution