grpc-loader: add method options in MethodDefinition

[hiepthai]

Add options and parsedOptions into MethodDefinition

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: hiepthai / name: Hiep Thai (a10471b)

[murgatroid99]

Can you send a corresponding PR to update this file with the new MethodDefinition fields?

[hiepthai]

Can you send a corresponding PR to update this file with the new MethodDefinition fields?

Yeah sure, please check this out grpc/proposal#328

[murgatroid99]

Both of those fields seem to contain the same information; what is the purpose of adding both of them? What does each one provide that is missing from the other one?

I am also still concerned about the types of these fields. any is a very broad type, and I would much prefer to be clear about exactly what is in these fields, especially because this type needs to be re-exported by the gRPC library that it's used with.

[hiepthai]

I updated both MRs with new changes:

• unify options and parsedOptions into options, wont expose raw options anymore
• add MethodOptions interface
• add test case

Waiting for your further feedback, thanks.

[murgatroid99]

I don't like what you're currently doing with the google.api.* option extensions. The fields are always in the MethodOptions type, but if I understand correctly, the user needs to explicitly depend on the package and explicitly put it in the includeDirs list for it to actually be provided. My preference would be to not single out any specific option extension for inclusion in these types, and to let users handle whatever extensions they want to use.

However, if there is a compelling reason for us to provide this specific extension directly, we should provide it consistently. That means moving google-proto-files to the dependencies list and adding it to the includeDirs option ourselves.

[murgatroid99]

I don't like the variance in these field types. I assume it comes from the options that were passed when loading the proto files. If possible, it would be better to have a specific type for this, if possible.

[hiepthai]

Could we leave custom extension type as unknown?

export interface MethodOptions {
  deprecated?: boolean;
  idempotency_level?: IdempotencyLevel|keyof typeof IdempotencyLevel;
  uninterpreted_option?: UninterpretedOption[];
  [k: string]: unknown
}

And clean up null values in other field types too, do you still need to separated type for UninterpretedOption values?

export enum IdempotencyLevel {
  IDEMPOTENCY_UNKNOWN = 0,
  NO_SIDE_EFFECTS = 1,
  IDEMPOTENT = 2
}

export interface NamePart {
  namePart: string;
  isExtension: boolean;
}

export interface UninterpretedOption {
  name?: NamePart[];
  identifierValue?: string;
  positiveIntValue?: number|Long|string;
  negativeIntValue?: number|Long|string;
  doubleValue?: number;
  stringValue?: Uint8Array|string;
  aggregateValue?: string;
}

export interface CustomHttpPattern {
  kind?: string;
  path?: string;
}

[murgatroid99]

Custom extension types as unknown seems like a good solution for that part to me. Removing the null values from the other fields is good too, but I'm more concerned about the number|Long|string part and similar. It would be better if we could declare in advance which specific type each field will have, either by fixing the corresponding protobuf options in advance or by having type conversion code on top of that. This also applies to stringValue and to idempotency_level.

Also, I just realized, what is the deal with the inconsistent field name format between MethodOptions and the other interfaces? In descriptor.proto, they all use snake_case.

[hiepthai]

Hi, sorry for this late response.

I updated & cleaned up MethodOptions and other related interfaces. As protobufjs won't handle 64-bit integers in method options, it will output just number at the moment.

I also updated new test cases; please check and let me know if you find any other issues.

[murgatroid99]

I don't like having these fields as optional and/or nullable. deprecated can coalesce to false, idempotency_level can coalesce to the default value, and uninterpreted_option can coalesce to []. Similarly, idempotency_level can be narrowed to either IdempotencyLevel or keyof typeof IdempotencyLevel. In all of these cases, if the value provided by protobuf.js has a wider type, it should be explicitly narrowed in code in this library.

[hiepthai]

I cleaned up these nullable values, and add a new test to make sure that protobuf.js won't have any fallback.

[murgatroid99]

These fields should also not be optional, and idempotency_level should be exactly one of IdempotencyLevel and keyof typeof IdempotencyLevel. My goal here is for values provided by this library to the user to have the narrowest types possible, and we can make these narrower because we control the code that returns them. We can explicitly set a value for each field that is not already set. We can convert idempotency_level to the chosen type if it has the wrong type.

[andoshin11]

@murgatroid99 @hiepthai

been waiting for this feature to come out :)
What's the status ?

[n0v1]

If it's ok for @hiepthai, I can try to continue work on this PR.

Two questions mainly to @murgatroid99:

1. Regarding "narrowest types possible": If I understand you correctly, e.g. for option deprecated you would like to see just boolean as type. This means if this option is not explicitly set to true inside the proto file it will be returned as false by proto-loader. So we loose the information whether the option was actually set or not. Not set is interpreted the same as explicitly set to false. This is probably fine for most use cases but there might be some which would benefit from an exact representation of the proto file. E.g. for documentation-centric use cases. Is it acceptable to loose this precision or do we want to differenciate between not set/set?
2. What code controls if an option is interpreted (like deprecated and idempotency_level) or not (like (google.api.http))? Is this predefined by protobufjs? I'm asking because I think google.api.http is one of the most widely used options and it could make sense to make it easier to use.

[murgatroid99]

1. My assumption here is that protobuf options are handled the same way as protobuf message fields, meaning that the absence of a field is considered equivalent to setting it to the default value. I don't see any downside of doing that here: there's no meaningful difference between not explicitly deprecated and explicitly not deprecated, and there's no meaningful difference between unspecified idempotency_level and IDEMPOTENCY_UNKNOWN.
2. This distinction is in the official option definitions in descriptor.proto. The options officially defined by protobuf are "interpreted", and all others are "uninterpreted".

[n0v1]

@murgatroid99, I created PR #2711 as a follow-up to this PR which tries to address your comments.

[murgatroid99]

Superseded by #2711.