Make sure we understand how references to messages outside the event-specific proto are handled

Question

Make sure we understand how references to messages outside the event-specific proto are handled

Closed this issue 7 months ago · 9 comments

Our event protos aren't self-contained: they can refer to protos within https://github.com/googleapis/google-cloudevents/tree/master/third_party/googleapis.

It would be useful to understand the impact of any references in terms of generated classes.

Proposed test - none of which would be committed:

Add fields of type google.rpc.ErrorInfo in three places:
- Two fields within google.events.cloud.cloudbuild.v1.RepoSource
- One field within google.events.cloud.cloudbuild.v1.StorageSource
- One field within google.events.cloud.storage.v1.StorageObjectData
Regenerate the schema
Regenerate any languages we're interested in based on that schema
- I'd suggest at least Java and Go

This allows us to check:

How the class for ErrorInfo is named (and is that a usable name)
Whether the class is reused when the fields are in the same message (RepoSource)
Whether the class is reused when the fields are in the same JSON schema (Cloud Build)
Interoperability across event types (Storage vs Cloud Build)

When I've got schema generation working on my (Windows) machine, I can do this test. I'll record results within this issue; it's not yet clear what the next steps will be. (We may want to document the results may prominently; we may want to change schema generation; we may not need to do anything.)

Answer 1 · 2021-03-11T12:30:09.000Z

Okay, I've now performed the test.

Fields added with build_repo_error, build_storage_error and storage_error (in the three places mentioned above).

Here's the change in schemas:

##In BuildEventData.json:

In definitions/RepoSource/properties there's a new entry:

        "buildRepoError": {
          "$ref": "#/definitions/ErrorInfo",
          "additionalProperties": true,
          "description": "An ErrorInfo in RepoSource"
        }

Likewise in definitions/StorageSource/properties:

        "buildStorageError": {
          "$ref": "#/definitions/ErrorInfo",
          "additionalProperties": true,
          "description": "An ErrorInfo in StorageSource"
        }

Then in definitions/ErrorInfo we have:

    "ErrorInfo": {
      "properties": {
        "reason": {
          "type": "string",
          "description": "The reason of the error. This is a constant value that identifies the\n proximate cause of the error. Error reasons are unique within a particular\n domain of errors. This should be at most 63 characters and match\n /[A-Z0-9_]+/."
        },
        "domain": {
          "type": "string",
          "description": "The logical grouping to which the \"reason\" belongs.  Often \"domain\" will\n contain the registered service name of the tool or product that is the\n source of the error. Example: \"pubsub.googleapis.com\". If the error is\n common across many APIs, the first segment of the example above will be\n omitted.  The value will be, \"googleapis.com\"."
        },
        "metadata": {
          "additionalProperties": {
            "type": "string"
          },
          "type": "object",
          "description": "Additional structured details about this error.\n\n Keys should match /[a-zA-Z0-9-_]/ and be limited to 64 characters in\n length. When identifying the current value of an exceeded limit, the units\n should be contained in the key, not the value.  For example, rather than\n {\"instanceLimit\": \"100/request\"}, should be returned as,\n {\"instanceLimitPerRequest\": \"100\"}, if the client exceeds the number of\n instances that can be created in a single (batch) request."
        }
      },
      "additionalProperties": true,
      "type": "object",
      "description": "Describes the cause of the error with structured details.\n\n Example of an error when contacting the \"pubsub.googleapis.com\" API when it\n is not enabled:\n     { \"reason\": \"API_DISABLED\"\n       \"domain\": \"googleapis.com\"\n       \"metadata\": {\n         \"resource\": \"projects/123\",\n         \"service\": \"pubsub.googleapis.com\"\n       }\n     }\n This response indicates that the pubsub.googleapis.com API is not enabled.\n\n Example of an error that is returned when attempting to create a Spanner\n instance in a region that is out of stock:\n     { \"reason\": \"STOCKOUT\"\n       \"domain\": \"spanner.googleapis.com\",\n       \"metadata\": {\n         \"availableRegions\": \"us-central1,us-east2\"\n       }\n     }",
      "id": "google.rpc.ErrorInfo"
    }

Compare that with StorageObjectData.json where the definition is inlined:

    "storageError": {
      "properties": {
        "reason": {
          "type": "string",
          "description": "The reason of the error. This is a constant value that identifies the\n proximate cause of the error. Error reasons are unique within a particular\n domain of errors. This should be at most 63 characters and match\n /[A-Z0-9_]+/."
        },
        "domain": {
          "type": "string",
          "description": "The logical grouping to which the \"reason\" belongs.  Often \"domain\" will\n contain the registered service name of the tool or product that is the\n source of the error. Example: \"pubsub.googleapis.com\". If the error is\n common across many APIs, the first segment of the example above will be\n omitted.  The value will be, \"googleapis.com\"."
        },
        "metadata": {
          "additionalProperties": {
            "type": "string"
          },
          "type": "object",
          "description": "Additional structured details about this error.\n\n Keys should match /[a-zA-Z0-9-_]/ and be limited to 64 characters in\n length. When identifying the current value of an exceeded limit, the units\n should be contained in the key, not the value.  For example, rather than\n {\"instanceLimit\": \"100/request\"}, should be returned as,\n {\"instanceLimitPerRequest\": \"100\"}, if the client exceeds the number of\n instances that can be created in a single (batch) request."
        }
      },
      "additionalProperties": true,
      "type": "object",
      "description": "An ErrorInfo in StorageObjectData"
    },

Running the Java tools/gen.sh script doesn't work cleanly for me, but it does perform the generation... which ends up with three classes:

BuildStorageError.java
BuildRepoError.java
StorageError.java

So even though in CloudBuild, the JSON schema refers to a single definition from two places, we still end up with two classes generated from the same proto... and a third class for Storage.

Assigning to Grant for consideration of the way forward.

Answer 2 · 2021-03-24T20:45:43.000Z

I'm not sure how to triage this issue. With JSON schema, each schema is self-contained and can iterate and evolve separately from other schemas. One of the things we can look into in the future is if there are ways to re-use schemas/code per language library if beneficial.

@jskeet What do we want to change, or what is the developer issue we're trying to raise up here?

Answer 3 · 2021-03-24T20:50:04.000Z

@grant: I would suggest that we should ask each language owner to consider the developer experience and decide whether it's sufficient or not.

The example I gave in Java (with two classes for the same event representing the same proto) doesn't feel like the experience we want for customers. Between different events there's a bit more room for understanding the duplication, although if they're within the same library I'd say that's still pretty deeply suboptimal.

Are you happy to coordinate that information-gathering?

Answer 4 · 2021-04-09T19:07:39.000Z

@grant: I would suggest that we should ask each language owner to consider the developer experience and decide whether it's sufficient or not.

The example I gave in Java (with two classes for the same event representing the same proto) doesn't feel like the experience we want for customers. Between different events there's a bit more room for understanding the duplication, although if they're within the same library I'd say that's still pretty deeply suboptimal.

Are you happy to coordinate that information-gathering?

That sounds like a good plan. Looking deeper into the example, I can imagine that we have common language definitions for a set of common protos. We're going to need to look into each language. The team may be changing priorities so I'm not sure about timing right now for when I can do that.

Answer 5 · 2021-10-15T00:34:20.000Z

@jskeet I did a bit more research looking at every event type.

There are a couple of instances of references outside the event-specific proto:

Products:

Audit Logs
Firestore

Non-event specific types:

google.api.MonitoredResource
google.rpc.Status
google.rpc.context.AttributeContext.Auth
google.rpc.context.AttributeContext.Peer
google.rpc.context.AttributeContext.Request
google.rpc.context.AttributeContext.Resource
google.type.LatLng

I don't think there are common event types right now between protos.

Right now we just inline the types within the JSON schema. We could create a set of common types for the above types, but right now they wouldn't have any effect.

Answer 6 · 2021-10-15T07:06:18.000Z

We could create a set of common types for the above types, but right now they wouldn't have any effect.

And that's what I'm suggesting should change.

Inlining "external" protos - and doing so on every occurrence within a proto (as per my test) really doesn't feel like a good customer experience. It makes it much harder to reuse code in statically-typed languages, for example. Again, I think this would be worth asking individual language owners about.

Answer 7 · 2022-02-03T19:57:59.000Z

Chatted with @grant, who said he's no longer actively working on this.

@jskeet is this something you can follow up on (scheduling language owner sync), or should we close for lack of {knowledge, resources}?

Answer 8 · 2022-02-03T20:01:03.000Z

We shouldn't close it. It should be part of our discussions about the DevX for events. (Those discussions are ongoing, they've just been a bit behind-the-scenes.) I'm happy to keep it assigned to me.

Answer 9 · 2024-04-08T14:03:12.000Z

I don't think we're using the JSON schema any more, so I'll close this as basically being obsolete.