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

Generated path templates should include all valid patterns for child_type resource references #570

Open
sushicw opened this issue Jun 18, 2020 · 2 comments
Assignees
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@sushicw
Copy link

sushicw commented Jun 18, 2020

In #488, we noticed that no path template exists for projects/{project}/locations/{location}, even though this is a valid input for the parent field on DLP API calls.

Based on https://google.aip.dev/122 and https://google.aip.dev/123, it seems like a path template should have been generated for the DLP API.

What I'd naively expect to happen, is that given the following in the DLP proto:

message InspectTemplate {
  option (google.api.resource) = {
    type: "dlp.googleapis.com/InspectTemplate"
    pattern: "organizations/{organization}/inspectTemplates/{inspect_template}"
    pattern: "projects/{project}/inspectTemplates/{inspect_template}"
    pattern: "organizations/{organization}/locations/{location}/inspectTemplates/{inspect_template}"
    pattern: "projects/{project}/locations/{location}/inspectTemplates/{inspect_template}"
  };
  
message CreateInspectTemplateRequest {
  // Parent resource name.
  // - Format:projects/[PROJECT-ID]
  // - Format:organizations/[ORGANIZATION-ID]
  // - Format:projects/[PROJECT-ID]/locations/[LOCATION-ID]
  // - Format:organizations/[ORGANIZATION-ID]/locations/[LOCATION-ID]
  string parent = 1 [
    (google.api.authz).permissions = "dlp.inspectTemplates.create",
    (google.api.field_behavior) = REQUIRED,
    (google.api.resource_reference).child_type =
        "dlp.googleapis.com/InspectTemplate",
    (google.api.field_auditing).directive = "AUDIT"
  ];

I would end up with the following NodeJS path templates:

projectPathTemplate: new this._gaxModule.PathTemplate(
'projects/{project}'
),
projectLocationPathTemplate: new this._gaxModule.PathTemplate(
'projects/{project}/locations/{location}'
),
organizationPathTemplate: new this._gaxModule.PathTemplate(
'organizations/{organization}'
),
organizationLocationPathTemplate: new this._gaxModule.PathTemplate(
'organizations/{organization}/locations/{location}'
),

As far as I can tell, this conforms with the API guidance linked above, and it seems like this should result in usable path templates for all of the valid forms of parent. However, we don't see all the templates described above. (Current set of generated path templates)

As the current logic was explained to me, path templates are only generated if they are directly referenced in a resource. Since this doesn't happen (and there's no reason in the API definition that it needs to happen), no path templates are generated. The existing projectPathTemplate that does exist occurs because many of our APIs (like CreateDlpJob) annotate with (google.api.resource_reference) = { type: "cloudresourcemanager.googleapis.com/Project", which is the kind of direct reference that the current generation logic is looking for.

Request:
Should the client generation code create templates for all of the patterns in the message being used in a (google.api.resource_reference).child_type annotation? (With the last segment trimmed, of course).
Or, as put by @alexander-fenster :

so long story short, you suggest that if we have a child_type, we generate helpers for all subtemplates no matter what?

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Jun 19, 2020
@alexander-fenster alexander-fenster added priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Jun 22, 2020
@alexander-fenster alexander-fenster self-assigned this Jun 22, 2020
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 25, 2020
@alexander-fenster alexander-fenster added priority: p2 Moderately-important priority. Fix may not be included in next release. and removed 🚨 This issue needs some love. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Jul 9, 2020
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Oct 7, 2020
@arithmetic1728
Copy link

any updates as we are out of slo

@alexander-fenster
Copy link
Contributor

Not in my plan for this month, and no real complaints from users - I'll downgrade this to a feature request.

@alexander-fenster alexander-fenster added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. and removed priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 13, 2020
@alexander-fenster alexander-fenster removed their assignment Feb 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

No branches or pull requests

5 participants