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

AIP 132: HTTP URI should include a parent variable #1285

Open
loeffel-io opened this issue Nov 8, 2023 · 3 comments
Open

AIP 132: HTTP URI should include a parent variable #1285

loeffel-io opened this issue Nov 8, 2023 · 3 comments

Comments

@loeffel-io
Copy link
Contributor

loeffel-io commented Nov 8, 2023

Version: 1.59.1

Given this method

  rpc ListMediaTypes(ListMediaTypesRequest) returns (ListMediaTypesResponse) {
    option (google.api.http) = {
      get: "/v1/mediaTypes",
      additional_bindings: [
        {
          get: "/v1/{parent=contents/*}/mediaTypes"
        },
        {
          get: "/v1/{parent=contentGroups/*}/mediaTypes"
        }
      ],
    };
    option (google.api.method_signature) = "parent";
  }

the aip-linter throws message: HTTP URI should include a parent variable while at AIP-127 under Multiple URI bindings this looks allowed or here https://github.com/googleapis/googleapis/blob/master/google/cloud/resourcemanager/v3/tag_keys.proto#L45C19-L45C37

is this expected? Thank you 🙏

@loeffel-io
Copy link
Contributor Author

loeffel-io commented Feb 15, 2024

ping @noahdietz 🙏

same issue here:

  rpc ListEntitlements(ListEntitlementsRequest) returns (ListEntitlementsResponse) {
    option (google.api.http) = {
      get: "/v1/{parent=users/*}/entitlements"
      additional_bindings: {
        get: "/v1/entitlements"
      }
    };
    option (google.api.method_signature) = "parent";
  }

@noahdietz
Copy link
Collaborator

this looks allowed or here
https://github.com/googleapis/googleapis/blob/master/google/cloud/resourcemanager/v3/tag_keys.proto#L45C19-L45C37

is this expected? Thank you 🙏

In looking at the internal changes for this example, it looks like it predates the linter. If this file were linted today it would like produce a finding.

I think in general the linter does not handle "top level" resources all that well, since in Google Cloud, there are very few examples (everything is parented by a Project, Org, Folder, etc.). It is also not super common to have a resource that is both top-level and potentially parented by another resource. For these reasons, exempting a single RPC via the disablement comment or config is preferable to relaxing the implementation and potentially missing out on a real finding.

Can you share more of the example/case you have i.e., the request message, resource definition?

One solution would be to perhaps tweak this rule to be more relaxed if the string parent field is not google.api.field_behavior = REQUIRED - which it is for Standard List in all cases except for those mentioned above.

That said, if one doesn't want the parent field to be required, but still wants an additional binding with parent, what will warn them if that is missing? That's the tricky part...

@loeffel-io
Copy link
Contributor Author

Yes, its pretty tricky! Thank you for your detailed answer

Here is the full proto file for now:

syntax = "proto3";

package mindful.earth.entitlement.v1;

option go_package = "github.com/mindful-hq/earth-payment-service-proto/mindful/earth/entitlement/v1;entitlement";

import "validate/validate.proto";
import "google/protobuf/timestamp.proto";
import "google/api/annotations.proto";
import "google/api/field_behavior.proto";
import "google/api/resource.proto";
import "google/api/client.proto";
import "mindful/global/order/v1/order.proto";
import "google/protobuf/empty.proto";
import "google/protobuf/descriptor.proto";

// Service for all entitlement resources
service EntitlementService {
  // Used to get a single entitlement.
  rpc GetEntitlement(GetEntitlementRequest) returns (Entitlement) {
    option (google.api.http) = {
      get: "/v1/{name=users/*/entitlements/*}"
      additional_bindings: {
        get: "/v1/{name=entitlements/*}"
      }
    };
    option (google.api.method_signature) = "name";
  }
  // Used to get multiple entitlements.
  rpc BatchGetEntitlements(BatchGetEntitlementsRequest) returns (BatchGetEntitlementsResponse) {
    option (google.api.http) = {
      get: "/v1/{parent=users/*}/entitlements:batchGet"
      additional_bindings: {
        get: "/v1/entitlements:batchGet"
      }
    };
  }
  // Used to get multiple entitlements.
  // (-- api-linter: core::0132::http-uri-parent=disabled
  //     aip.dev/not-precedent: https://github.com/googleapis/api-linter/issues/1285. --)
  rpc ListEntitlements(ListEntitlementsRequest) returns (ListEntitlementsResponse) {
    option (google.api.http) = {
      get: "/v1/{parent=users/*}/entitlements"
      additional_bindings: {
        get: "/v1/entitlements"
      }
    };
    option (google.api.method_signature) = "parent";
  }
}

// Represents the entitlement resource
message Entitlement {
  option (google.api.resource) = {
    type: "payment.mindful.com/Entitlement",
    pattern: "users/{user}/entitlements/{entitlement}",
    pattern: "entitlements/{entitlement}",
    singular: "entitlement",
    plural: "entitlements"
  };
  // The resource name.
  // Format: entitlements/{entitlement}
  // Format: users/{user}/entitlements/{entitlement}
  string name = 1 [
    (google.api.field_behavior) = IDENTIFIER,
    (validate.rules).string = {pattern: "^(users\\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}\\/)?entitlements\\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$"}
  ];
  // The resource description
  string description = 2 [
    (google.api.field_behavior) = REQUIRED
  ];
  // The resource create time
  google.protobuf.Timestamp create_time = 3 [
    (google.api.field_behavior) = OUTPUT_ONLY
  ];
  // The resource update time
  google.protobuf.Timestamp update_time = 4 [
    (google.api.field_behavior) = OUTPUT_ONLY
  ];
  // This checksum is computed by the server based on the value of other
  // fields, and may be sent on update and delete requests to ensure the
  // client has an up-to-date value before proceeding.
  string etag = 99 [
    (google.api.field_behavior) = REQUIRED
  ];
}

// Represents the get entitlement request
message GetEntitlementRequest{
  // The ressource name
  string name = 1 [
    (google.api.resource_reference) = {type: "payment.mindful.com/Entitlement"},
    (google.api.field_behavior) = REQUIRED,
    (validate.rules).string = {pattern: "^(users\\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}\\/)?entitlements\\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$"}
  ];
  // The ressource read masks
  repeated GET_ENTITLEMENT_REQUEST_READ_MASK read_mask = 2 [(google.api.field_behavior) = OPTIONAL];
}

// Represents the list entitlements request
message ListEntitlementsRequest{
  // The parent, which owns this collection of entitlements.
  // Format: users/{user}
  // (-- api-linter: core::0132::request-parent-behavior=disabled
  //     aip.dev/not-precedent: Parent is optional in case of additional bindings. --)
  string parent = 1 [
    (google.api.field_behavior) = OPTIONAL,
    (google.api.resource_reference) = {child_type: "payment.mindful.com/Entitlement"},
    (validate.rules).string = {ignore_empty: true, pattern: "^users\\/([a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}|-)$"}
  ];
  // The filters
  ListEntitlementsRequestFilter filter = 2 [(google.api.field_behavior) = OPTIONAL];
  // The order
  ListEntitlementsRequestOrder order_by = 3 [(google.api.field_behavior) = OPTIONAL];
  // The page size
  int32 page_size = 4 [
    (validate.rules).int32 = {ignore_empty: true, gte: 1, lte: 100},
    (google.api.field_behavior) = OPTIONAL
  ];
  // The page token
  string page_token = 5 [(google.api.field_behavior) = OPTIONAL];
  // the read masks
  repeated LIST_ENTITLEMENTS_REQUEST_READ_MASK read_mask = 6 [(google.api.field_behavior) = OPTIONAL];
}

// Represents the list entitlements response
message ListEntitlementsResponse{
  // The entitlement resources
  repeated Entitlement entitlements = 1;
  // The next page token
  string next_page_token = 2;
  // The total size
  int32 total_size = 3;
}

// Represents the batch get entitlements request
message BatchGetEntitlementsRequest{
  // The parent resource shared by all users being retrieved.
  // Format: users/{user}
  // If this is set, the parent of all of the users specified in `names`
  // must match this field.
  string parent = 1 [
    (google.api.resource_reference) = {child_type: "payment.mindful.com/User"},
    (google.api.field_behavior) = OPTIONAL,
    (validate.rules).string = {ignore_empty: true, pattern: "^users\\/([a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}|-)$"}
  ];
  // The ressource name
  repeated string names = 2 [
    (google.api.resource_reference) = {type: "payment.mindful.com/Entitlement"},
    (google.api.field_behavior) = REQUIRED,
    (validate.rules).repeated = {min_items: 1, max_items: 1000, items: {string: {pattern: "^(users\\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}\\/)?entitlements\\/[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{4}-[a-fA-F0-9]{12}$"}}}
  ];
  // The ressource read masks
  repeated BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK read_mask = 3 [(google.api.field_behavior) = OPTIONAL];
}

// Represents the batch get entitlements response
message BatchGetEntitlementsResponse{
  // The entitlements
  repeated Entitlement entitlements = 1;
}

// Represents the list entitlements request description filter
message ListEntitlementsRequestDescriptionFilter {
  // The description
  string description = 1 [
    (google.api.field_behavior) = REQUIRED
  ];
}

// Represents the list entitlements request filter
message ListEntitlementsRequestFilter{
  // The description filter
  optional ListEntitlementsRequestDescriptionFilter description = 1 [(google.api.field_behavior) = OPTIONAL];
}

// Represents the list request order
message ListEntitlementsRequestOrder {
  // The order type
  LIST_ENTITLEMENTS_REQUEST_ORDER_TYPE order_type = 1 [(google.api.field_behavior) = OPTIONAL];
  // The order sort
  mindful.global.order.v1.ORDER_SORT order_sort = 2 [(google.api.field_behavior) = OPTIONAL];
}

// Represents the internal list entitlements request page token
message ListEntitlementsRequestPageToken {
  // The hash
  // sha256 sum of filters and order
  string hash = 1 [
    (google.api.field_behavior) = REQUIRED,
    (validate.rules).string = {len: 64}
  ];
  // The expire time
  google.protobuf.Timestamp expire_time = 2 [
    (validate.rules).timestamp.required = true
  ];
  // The last entitlement id
  string entitlement_id = 3 [
    (google.api.field_behavior) = REQUIRED,
    (validate.rules).string.uuid = true
  ];
  // The last order value
  oneof order_value {
    option (validate.required) = true;
    // The order create time value
    google.protobuf.Timestamp create_time = 4;
    // The order update time value
    google.protobuf.Timestamp update_time = 5;
  }
}

// Represents the list entitlements request order type
enum LIST_ENTITLEMENTS_REQUEST_ORDER_TYPE {
  // The unspecified order type
  LIST_ENTITLEMENTS_REQUEST_ORDER_TYPE_UNSPECIFIED = 0;
  // The create time order type
  LIST_ENTITLEMENTS_REQUEST_ORDER_TYPE_CREATE_TIME = 1;
  // The update time order type
  LIST_ENTITLEMENTS_REQUEST_ORDER_TYPE_UPDATE_TIME = 2;
}

// Represents the get entitlement request read masks
enum GET_ENTITLEMENT_REQUEST_READ_MASK {
  // The unspecified read mask.
  // Equivalent to all
  GET_ENTITLEMENT_REQUEST_READ_MASK_UNSPECIFIED = 0;
  // The all read mask
  GET_ENTITLEMENT_REQUEST_READ_MASK_ALL = 1;
  // The name read mask
  GET_ENTITLEMENT_REQUEST_READ_MASK_NAME = 2;
  // The description read mask
  GET_ENTITLEMENT_REQUEST_READ_MASK_DESCRIPTION = 3;
  // The create time read mask
  GET_ENTITLEMENT_REQUEST_READ_MASK_CREATE_TIME = 4;
  // The update time read mask.
  GET_ENTITLEMENT_REQUEST_READ_MASK_UPDATE_TIME = 5;
}

// Represents the list user entitlements request read masks
enum LIST_ENTITLEMENTS_REQUEST_READ_MASK{
  // The unspecified read mask.
  // Equivalent to all
  LIST_ENTITLEMENTS_REQUEST_READ_MASK_UNSPECIFIED = 0;
  // The all read mask
  LIST_ENTITLEMENTS_REQUEST_READ_MASK_ALL = 1;
  // The name read mask
  LIST_ENTITLEMENTS_REQUEST_READ_MASK_NAME = 2;
  // The description read mask
  LIST_ENTITLEMENTS_REQUEST_READ_MASK_DESCRIPTION = 3;
  // The create time read mask.
  LIST_ENTITLEMENTS_REQUEST_READ_MASK_CREATE_TIME = 4;
  // The update time read mask.
  LIST_ENTITLEMENTS_REQUEST_READ_MASK_UPDATE_TIME = 5;
}

// Represents the batch get user entitlement request read masks
enum BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK {
  // The unspecified read mask.
  // Equivalent to all
  BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK_UNSPECIFIED = 0;
  // The all read mask
  BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK_ALL = 1;
  // The name read mask
  BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK_NAME = 2;
  // The description read mask
  BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK_DESCRIPTION = 3;
  // The create time read mask
  BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK_CREATE_TIME = 4;
  // The update time read mask.
  BATCH_GET_ENTITLEMENTS_REQUEST_READ_MASK_UPDATE_TIME = 5;
}

(We do handle filters and field masks different)

Here is the linter

sh_test(
    name = "entitlement_v1_api_linter",
    srcs = ["@com_github_mindful_hq_rules//:api_linter"],
    args = [
        "mindful/earth/entitlement/v1/entitlement.proto",
        "--set-exit-status",
        "--proto-path external/com_github_envoyproxy_protoc_gen_validate",
        "--proto-path external/com_github_mindful_hq_global_proto_v2",
        "--proto-path external/googleapis",
        "--disable-rule core::0191::java-package",
        "--disable-rule core::0191::java-multiple-files",
        "--disable-rule core::0191::java-outer-classname",
        "--disable-rule core::0157::request-read-mask-field",
        "--disable-rule core::0132::request-field-types",
        "--disable-rule core::0134::request-mask-field",
    ],
    data = [
        ":entitlement_v1_proto",
    ],
    visibility = ["//visibility:public"],
)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants