bigquery: don't return null schema#2510
bigquery: don't return null schema#2510pongad merged 1 commit intogoogleapis:masterfrom pongad:bq-null
Conversation
pongad
requested review from
garrettjonesgoogle and
vam-google
as code owners
googlebot
added
the
cla: yes
|
|
||
| // Don't compare strings directly; BigQuery might reorder. | ||
| String gotCsvContent = new String(storage.readAllBytes(BUCKET, EXTRACT_FILE), StandardCharsets.UTF_8); | ||
| Truth.assertThat(gotCsvContent.split("\n")).asList().containsExactly(CSV_CONTENT.split("\n")); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
| Table toPb() { | ||
| Table tablePb = new Table(); | ||
| if (schema != null) { | ||
| if (schema.getFields().size() != 0) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry for being late on this and blocking merge in an a semi-rude way, but most probably the correct way to solve this is to remove checkNotNull(schema) from com.google.cloud.bigquery.TableDefinition.Builder.setSchema(). (i.e. fix it form the "other end").
The arguments to support this:
-
There are other places where schema can be null (check
FieldValueList.get(String)method for example). -
In general builders usually accept nulls and do not enforce that kind of validation (check other builders). Builders should allow any intermediate state of things, since any state of builder is not final and it can be "fixed" before actual
build()is called. -
(This is the most important one) if a table does not have a schema and it is not returned from the server, we should be explicit about it (i.e. no schema - here is your
null). Faking schema, when server returns null introduces inconsistency between server state and client and can cause weird bugs / confusing behavior in the future.
| Table toPb() { | ||
| Table tablePb = new Table(); | ||
| if (schema != null) { | ||
| if (schema.getFields().size() != 0) { |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@vam-google For (2), AutoValue also rejects |
|
(3) You're right that allowing nulls is the most correct way to fix this. The BigQuery API represents a schema as an object (with just one property: fields). This means that there is a difference between a null schema and an empty schema, though I don't know of a case where an empty schema would be returned by the API. I suppose there could be a case in the future where it makes a difference (such as if another property besides fields was added to schema) |
|
@tswast From this issue it looks like BigQuery returns null schema if a table is created with no fields. If we allow null in client lib, is this something we can fix on the server side? |
|
What would we fix server-side? It's a problem with the way the API is designed so making schema non-nullable server-side would be a breaking change. |
|
@tswast Ah yes, sorry. I was being stupid. Shall we fix this by allowing user to set Schema to null then? |
|
Yeah, allowing null is the safest thing to do in regards to avoiding problems in the future due to a difference in how schemas are handled in the client versus the API. It does feel a bit yucky as a user, but better to err on the side of avoiding a disconnect between API representation and client library. |
Schema can be set to null in a response we get from server. However, we do not allow the user to set to null, causing confusion. This commit fixes this by allowing user to set null schema. Fixes #2477.
|
Thank you @tswast sounds good. I updated the PR. PTAL |
|
LGTM. |
Schema can be set to null in a response we get from server. However, we do not allow the user to set to null, causing confusion. This commit fixes this by allowing user to set null schema. Fixes googleapis#2477.
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
5 participants
Fixes #2477