DATAMONGO-759 - Render group operation without non synthetic fields corr... #73
Conversation
| * @return | ||
| */ | ||
| private int exposedFieldsCount() { | ||
| return originalFields.size() + syntheticFields.size(); |
There was a problem hiding this comment.
I think we have to look for the original fields only. Otherwise we eventually render a {} again as soon as someone adds a synthetic field to the group operation. Could be that we have to find a better name for what's currently isEmpty().
There was a problem hiding this comment.
Are you sure? ExposesField combines "synthetic" as well as "original" fields hence a method like exposedFieldsCount should consider synthetic as well as original fields.
In addition to that I added a test case to verify that this is appropriate.
There was a problem hiding this comment.
I am not arguing about the name. If that needs to be different, fine. Maybe the issue I see is more driven from the variable name used in GroupOperation. In this class we hold an instance of ExposedFields (which can hold both synthetic and non-synthetic fields) but only store synthetic fields in it. Without realizing the latter, simply calling ….isEmpty() creates the impression we check for whether there are no synthetic and non-synthetic fields present. So we're using a method here that semantically only makes sense, because we use the type in a certain way. I'd argue adding a hasNonSyntheticFields() method only checking the non-synthetic fields is semantically clearer - both on the type and on the client side using the type in a very special way.
There was a problem hiding this comment.
I choose to use exposesNoNonSyntheticFields() instead of hasNonSyntheticFields() in order to stay consistent with the other methods. Changed the name of nonSyntheticFields in GroupOperation to idFields to make their purpose more clear.
|
I changed isEmpty() to exposesNoFields() and added test cases to verify that a null group id is generated when no group-id fields are given but aggregation fields are present. |
…orrectly. Added isEmpty() method to ExposedFields to be able to test whether fields are going to be exposed or not. Adjusted the rendering of GroupOperations to generate null as group id (_id) when no fields are given.
GroupOperation gets the _id field now rendered as null if no group fields were added to the operation. Previously it was rendered as empty document (i.e. { }). While this was technically correct as well, we're now closer to what the MongoDB reference documentation describes.
Original pull request: #73.
GroupOperation gets the _id field now rendered as null if no group fields were added to the operation. Previously it was rendered as empty document (i.e. { }). While this was technically correct as well, we're now closer to what the MongoDB reference documentation describes.
Original pull request: #73.
...ectly.
Added isEmpty() method to ExposedFields to be able to test whether fields are going to be exposed or not. Adjusted the rendering of GroupOperations to generate null as group id (_id) when no fields are given.