SumAggregation missing ctor about elasticsearch-net HOT 7 CLOSED

I am OK with whatever you decide. I am just expressing personal opinion. 10x for the conversation

from elasticsearch-net.

Hi @mynkow,

the name is no longer part of the aggregation variant, so that part is expected to be missing.

The "field" constructor provides no real benefit over using the initializer syntax:

var agg = new SumAggregation { Field = field }

Is there a specific need for this constructor? Otherwise I'm going to document this change in the breaking changes list as I've definitely forgot to mention it so far 🙂

from elasticsearch-net.

hello @flobernd,

from coding perspective ctors indicate what minimum data (or combination) is required to have valid object for the rest of the flow. Is it valid to have a SumAggregation without a Field?

I see that the other properties are nullable but I think it is easier having a ctor.

from elasticsearch-net.

Hi @mynkow,

from coding perspective ctors indicate what minimum data (or combination) is required to have valid object for the rest of the flow

Yes, that's my understanding as well and I think the client should follow this pattern on most places (except for descriptors). Currently, the code base does not strictly implement nullability annotations. This is something I plan to improve in the future.

Is it valid to have a SumAggregation without a Field?

I was already wondering the same. According to our specification, Field can be null (this is as well one of the reasons for the code generator to not add field as a required constructor argument). I fired a quick query without specifying field and the response from the server was as follows:

Required one of fields [field, script], but none were specified.

This means, field can indeed by null, if script is used instead.

Based on this observation, I think the removal of the constructor is valid. Do you agree with that?

from elasticsearch-net.

Maybe the best solution is to have 2 constructors in this situation. One with the field and one with the script because these are the minimum requirements from the server. This way developers will get faster feedback instead of runtime error.

What do you think?

from elasticsearch-net.

I agree, that this would be a good design, but it's very complex to model in our specification. This would require us to include semantics in the specification in order to express that either X or Y is required. While this particular case would probably be relatively easy to implement, there are more complex cases where e.g. X is required only, if Y contains a certain value and many other conditions which might even be dependent on state (e.g. mapping on a specific index field).

A better idea might be to improve documentation and mention these kinds of conditions there.

from elasticsearch-net.

Thank you as well for providing feedback 🙂 I'm always happy to improve things, if possible.

from elasticsearch-net.