Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: accept null values in request bodies #1824

Merged
merged 1 commit into from Sep 17, 2019
Merged

fix!: accept null values in request bodies #1824

merged 1 commit into from Sep 17, 2019

Conversation

JustinBeckwith
Copy link
Contributor

@JustinBeckwith JustinBeckwith commented Sep 15, 2019

BREAKING CHANGE: This fixes #1559, and runs the generator.

The following APIs have been removed:

  • healthcare/v1alpha2

The following APIs have been added:

  • bigqueryconnection/v1beta1
  • binaryauthorization/v1
  • compute/alpha

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 15, 2019
@JustinBeckwith JustinBeckwith changed the title feat!: run the generator fix!: accept null values in request bodies Sep 15, 2019
@acchou
Copy link

acchou commented Nov 7, 2019

This change results in many APIs that were typed as T | undefined to now have type T | undefined | null. For example in cloudfunctions_v1:

    export interface Schema$ListFunctionsResponse {
        nextPageToken?: string | null;
    }

The nextPageToken? allows for an implicit undefined type, so the complete type is string | null | undefined. It's a bit odd to have both null and undefined as possible results in and of itself, but more troubling is that this is an argument to the list() function:

    export interface Params$Resource$Projects$Locations$Functions$List extends StandardParameters {
        /**
         * The value returned by the last `ListFunctionsResponse`; indicates that this is a continuation of a prior `ListFunctions` call, and that the system should return the next page of data.
         */
        pageToken?: string;
    }

Here, the input type for pageToken is string | undefined. Is the client supposed to sanitize the null value and turn it into undefined? Occam's razor would suggest that either nextPageToken or pageToken has the wrong type here.

There are many type changes, this is just one small example of what might lurk here.

@bcoe
Copy link
Contributor

bcoe commented Nov 10, 2019

@acchou are you bumping into any specific issues? this is an attempt to address the fact that some APIs do expect a null value to be populated, rather than undefined.

If you were to populate null for the pageToken, I would expect the upstream API to either disregards this, or throw an error.

@acchou
Copy link

acchou commented Nov 10, 2019

@acchou are you bumping into any specific issues? this is an attempt to address the fact that some APIs do expect a null value to be populated, rather than undefined.

Then these APIs should have undefined removed from their type (i.e. remove the ?)

If you were to populate null for the pageToken, I would expect the upstream API to either disregards this, or throw an error.

The page token is an opaque value, it is given by the return value of one call to the functions.list API and passed into a subsequent call. My issue is that the types for the output aren't compatible with the type for the input. Therefore as a user of this API I'm forced to decide how to handle null.

It's not a huge deal to work around this specific case, but it is odd that the APIs are defined such that I need to sanitize the value returned by changing null to undefined for you, even though it's supposed to be an opaque value to me.

The larger issue is that this change creates a fair amount of inconsistency in the API definitions w.r.t. use of null vs. undefined. This can cause client code to be quite messy, as it needs to navigate this inconsistency in many places.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Calendar Typescript Schema$EventDateTime.date type missing null
4 participants