Return an empty collection instead of null.

[aozarov]

Some of the the resource metadata properties are collections/maps.

I think currently we are not consistent and in some cases we return null if there is no value (e.g.
BlobInfo and BucketInfo) and in some cases we return an empty collection (e.g. ProjectInfo).

My preference would be to never return null for a collection as I think the value of being able to
distinct the case of "unassigned" and "assigned and empty" is very small and does not justify
the cost for checking for null and empty.

In any case, I think we should be consistent.

/cc @mziccard @ajkannan @mderka

[ajkannan]

+1 to never returning null. Even when users say "get" or "list" and leave out metatdata properties using field options, I think it's reasonable to have empty collections instead of null collections.

[mderka]

Agreed to never returning null for collections received as Arie suggests.

However, with fields specification, there is a difference between fields being empty and fields being null. Null means that I did not filter fields. Empty means that I specifically asked for no fields. I believe that services allow this. A reason for asking for no fields is for example to simulate an exist() function. I can want to shoot a request with empty fields just to see if I get 200 response meaning found, or null/404 for not founds.

[mziccard]

While I agree that in general that we should be consistent and favor empty lists over null I would be careful when it comes to fields selection.
For instance in compute a disk has a list of attached instances. An empty list might mean that the disk is not attached to any instance. If the attachedInstances field is not selected through the DiskOption.fields() option returning an empty list might be counter-intuitive.

However, one could also argue that now we return null for both the "non instances attached" and "field not selected" cases.

[mderka]

I would actually prefer null to empty list in the scenario of attached disks described by @mziccard as the empty list has special and significant meaning.

[aozarov]

@mderka Yes, I was regarding return values and therefore I think this issue should not affect the way we
currently handle field selection (using vararg). Did I get your comment wrong?

@mziccard You are raising an interesting case of an incomplete resource (due to field selection).
Which is similar (though not identical) to when constructing a source[Info] with partial fields.

However, as you mentioned, returning null still masks what I consider as a "user-error".
I agree with @mderka that null is more intuitive than an empty collection in that case but
it is still a user-error and can lead to undesired result.

We could, with more work, identify the case of accessing a field that is not populated and throw
IllegalStateException (which we should clearly document).

Thoughts?

[mderka]

@aozarov No, I actually understood @ajkannan's comment wrong. I take this back, all clear.

It probably is a user error. Can you be more specific about how to verifying unpopulated fields. Do you mean something like holding values in Optional<> (I guess Guava's since we are not in Java 8)?

[aozarov]

I think there are various ways to achieve that but as it requires even more work than just 'null collection'` to 'empty collection' search and replace I want to make sure we are all on board with that.

One way of implementing that is to make the resource's builder do the bookkeeping of what was set.
This could be done by using Set (where field could be the same enum we use for the resource
field selection in the service interface). The builder adds to the Set per setter method.
When constructed the resource can use that set to check if exists per getter method.
We can also change the fromPb of the resource to accept that set and to pass it along to the builder.

[ajkannan]

My first choice is to throw an exception on getters for fields that aren't set and have an "isSet" method that people can use to check if a field wasn't set.

I don't think this would lead to too much more checking on the user's side because specifying fields is an optimization, and users who specify fields are likely to also know which fields they're selecting, and thus can avoid lots of isSet checks. On the other hand, if they don't know whether a field is set for an object (perhaps they have some collection of objects obtained from different requests with different fields), they would have to check anyways for null/default values, and using the isSet method makes that scenario cleaner.

[mziccard]

My first choice is to throw an exception on getters for fields that aren't set and have an "isSet" method that people can use to check if a field wasn't set.

This approach is sound and it's the right one if we want to go this way. My question is: is this worth it? Do we think that this approach is more usable than simple returning null for both cases (fields not selected and field not set)? I don't have a strong opinion but I would be careful in going this way as it will probably require a lot of changes.

Also XXXField enums we are using now match JSON fields but do not necessarily match a class field. This might be tricky to handle in some cases.

[mderka]

The suggested unified approach to returning empty collection instead of does not really make sense to me without IllegalStateException. It probably will be a lot of work but I cannot assess it in its entirety (I have only limited idea about the complexity of BigQuery, Storage and other modules).

I am not sure that I exactly follow the JSON issue. I think that these changes would be on the level of builders, constructors and toPb/fromPb methods, which is above JSON parsing. What am I missing?

[aozarov]

I don't have a strong opinion but I would be careful in going this way as it will probably require a lot of changes.

I do think this would be a better API as it can potentially eliminate user error dealing with wrong data.

However I agree that if we decide not to do it (and I am OK with that) than we should be consistent (which means some change) and in that case I agree that null for unassigned collection property is more appropriate.

We should still make sure that any non-getter method that return collection should never
return null.

Also XXXField enums we are using now match JSON fields but do not necessarily match a class field. This might be tricky to handle in some cases.

Is there any reason for not calling the XXXField enum value as the field name (maybe there is a good
reason for that, but in general I don't think that is desired).
In any case, the enum is just one option (and could also be internal, providing hasXXX per field instead
of a generic hasField(xxx) though less desired).

[mziccard]

Is there any reason for not calling the XXXField enum value as the field name (maybe there is a good
reason for that, but in general I don't think that is desired).

Consider the bigquery case where we have TableInfo which has a Definition field. Definition wraps some different JSON fields (e.g. schema, numBytes, numRows, streamingBuffer, etc.). With current XXXField enums the user can select only some of the JSON fields (e.g. only the schema) and the definition object will still be created with partial data. If we named XXXField enums after class's fields we would have a less fine granularity in field selection.

In any case, the enum is just one option (and could also be internal, providing hasXXX per field instead
of a generic hasField(xxx) though less desired).

Yes I agree this can be done (even still re-using XXXField somehow). I was just pointing out that is not as simple as one-to-one correspondence.