Deal with null QueryExpression and Projections in DataFindRequest better

[alechenninger]

Few options:

• Accept a QueryExpression and Projection(s) in constructor. This prevents the DataFindRequest from being allow to be in an invalid state which is a bad practice for constructors (allowing objects which will never actually work without more configuration).
• Throw something other than NPE if either are omitted (but this is still failing at runtime which is bad IMHO)
• Use a default QueryExpression and Projection if either is omitted (then you could also implement the first suggestion)

As an aside, this is why the Builder pattern is much prefered than mutable objects:

• Immutable objects are always +1
• You prevent objects being in invalid state
• You get "named parameters" from chainable methods in a Builder (some call this "fluent") so the code is more readable

So I guess I'm suggesting IMHO moving all of these requests to a builder pattern where the final object is immutable and guaranteed valid, but ultimately anything other than throwing an NPE would be nice.

[dcrissman]

@alechenninger is this still a concern?

[alechenninger]

Can you still create requests in an invalid state?

In other words, as a good design practice, I'd like to be able to create a request and see what is required from its constructor, and if something is omitted, it has a reasonable default. I don't want to find out I missed something vital only once I go to integration test it with a running lightblue API.

My 2c

[dcrissman]

Accept a QueryExpression and Projection(s) in constructor. This prevents the DataFindRequest from being allow to be in an invalid state which is a bad practice for constructors (allowing objects which will never actually work without more configuration).

I agree that a constructor shouldn't leave the object in an incomplete state (an issue I have with a lot of DI frameworks). I do like the expression style methods we currently have. So yeah, a builder would be the correct way to deal with this.

Throw something other than NPE if either are omitted (but this is still failing at runtime which is bad IMHO)

You shouldn't get a RuntimeException any more, but you will still get an exception from lightblue in the form of a LightblueResponseException.

Use a default QueryExpression and Projection if either is omitted (then you could also implement the first suggestion)

• We might be able to get away with a default Projection for a recursive return everything, but I don't like that idea and think the client really should be providing this for each execution.
• A default Query I feel is a bad idea.

[alechenninger]

I only suggested defaults as a fallback if you didn't make it explicit was required by a constructor or some other means.

If requests are able to be created invalid still, then we should at least throw an exception client side for obvious problems rather than waiting for lightblue to respond with an error. That is, if you're missing a projection for a query where it is required, throw an error client side like, InvalidRequestException or some such thing and explain what the problem is.

[alechenninger]

In other words, when I say failing at runtime is bad, it's only bad relative to failing at compile time. In other words

Catching failure at compile time (via combination of constructor forcing arguments and/or defaults) > Catching failure at runtime client side > Catching failure at runtime lightblue side

Waiting til it gets to lightblue is a bad time to find out you have an otherwise detectable bug in your query code basically.

[josh-cain]

Can I +1 this? #257 happened to me because I can construct invalid queries. As someone who has been working with LB for about a week now, I can say that a fluent builder that guaranteed a correct query would be fantastic. If you're not well-versed in the query syntax and internals there's plenty of rope out there with which it's easy to hang yourself.