api-linter

api-linter copied to clipboard

The implemented rule for AIP124 enforces that if a resource is referenced using google.api.resource_reference annotation, the referenced resource must be in the same package as the one referencing it. Otherwise, the linter will lint it as "resource not found".

Why is this implemented this way ?? This requirement is not mentioned in AIP124.

I think an explanation for this should be given in AIP124 or the requirement to be in the same package removed from the implementation of the rule.

Just to make it a bit clearer, here's an example. If we have 2 proto files, dep.proto, and leaf.proto (taken from the testing inside rule 124) like:

"dep.proto"

import "google/api/resource.proto";
package AAA
message Book {option (google.api.resource) = {
    type: "library.googleapis.com/Book"
}

"leaf.proto": `

import "google/api/resource.proto";
import "dep.proto";
package BBB
message Foo {
    string book = 1 [(google.api.resource_reference).{{.Field}} = "{{.Ref}}"];
}

AAA must be equal to BBB otherwise, the resource will be lint as not found.

This is an infrastructure limitation with client libraries (and one that it would be nice to fix). Leaving this open to add something here.

Could you elaborate a bit on what you mean by "an infrastructure limitation with client libraries" ? I am not sure about what you mean by this.

Looking at the code for rule 124, I understand that at the time of getting the dependencies of a file to validate a reference, it forces the dependency to be in the same package as the original resource: link

func getDependencies(file *desc.FileDescriptor, pkg string) map[string]*desc.FileDescriptor {
        answer := map[string]*desc.FileDescriptor{file.GetName(): file}
	for _, f := range file.GetDependencies() {
		if _, found := answer[f.GetName()]; !found && f.GetPackage() == pkg {
			answer[f.GetName()] = f
			for name, f2 := range getDependencies(f, pkg) {
				answer[name] = f2
			}
		}
	}
	return answer
}

Wouldn't it be enough to change the expression !found && f.GetPackage() == pkg for just !found ? what am i missing?

Sorry for the ping @lukesneeringer, but need to clarify something that is not making sense.

Release 1.3 separated this rule into two different rules, one for reference validation and one for ensuring that references point to resources in their package. This way, same package validation can be deactivated if needed, while leaving reference validation active. Thats OK, makes sense.

From your previous explanation:

This is an infrastructure limitation with client libraries (and one that it would be nice to fix).

I though same package validation was just because of infrastructure limitations, not because AIPs describe it this way. However i can see in docs/rules/0124

This rule enforces that resource reference annotations refer resources defined in the same package, as described in AIP-124.

But when I go to AIP-124 I cant find anything about the need for resources to be in the same package as the fields referencing them. Moreover, in my opinion, being able to point to resources in different packages should be a feature for resource oriented designs.

Is docs/rules/0124 wrong when pointing to AIP-124? or Is the AIP-124 documentation missing an explanation on why resources and fields referencing them need to be on the same package? or am I not understanding something here?

This is an infrastructure limitation with client libraries (and one that it would be nice to fix).

Does this mean that this is not considered a bug in the linter, but rather an API design limitation on how resource references can be used?

We have resource references across package boundaries, and are currently unable to apply this linting rule to our API.

It is of course easy to just disable this rule, but would be useful if you could help clarify how resource references are meant to be used correctly.

What @odsod said is correct.

The issue is that the code generators that generate client libraries from protos (e.g. see Python, Java, etc.) generate a single package and only that package.[^1] If you reference a resource defined elsewhere, they can not read the resource to determine its pattern, etc.

The workaround for now is to use the google.api.resource_definition file annotation in order to "fake" the definition in your own package. Obviously, you can ignore this if you do not need client library generation.

We do have an in-progress plan to fix this, at which point this rule can be retired.

[^1]: For the most part. Some nuances apply.

@lukesneeringer We've been running with this rule switched off for a while, but are looking into what it would mean to switch it on. (For the purpose of being able to reliably look up resource name patterns via reflection).

Right now, it seems the linter not only requires the resource definition to be in the same package, but even the same file.

Just want to check before I jump the gun here - do you think it would be a good idea to patch the GetResourceDefinitions method to iterate through all the files in the same package as the provided file?

That should allow the linter to only trigger when there is no resource definition in any file in the same package.

Just want to check before I jump the gun here - do you think it would be a good idea to patch the GetResourceDefinitions method to iterate through all the files in the same package as the provided file?

That should allow the linter to only trigger when there is no resource definition in any file in the same package.

Yeah this sounds right and makes the most sense with what our code generators support (or should support) today with regards to scoping.

I'm not sure if you are still interested in contributing something, but I'd be willing to review it!

I was just looking at this again. There is larger technical limitation in the linter that will take some work to build out. The problem is that the context of an individual rule running doesn't contain all files in the initial linter request.

Furthermore, it may be that the proto files provided to the linter are incomplete e.g. only those that were changed + their direct dependencies, but not necessarily all of the "neighboring" files in the package.

As such, we cannot reliably resolve a resource definition on any given run. This is probably an OK false-positive because it would already be happening in with the code we have today. So we help some situations, but not all.