[db-macros] support for children without names in lookup_resource!
#7508
Conversation
Presently, the `lookup_resource!` macro generates code that will not compile when a child resource has `lookup_by_name = false` in its `lookup_resource!` macro invocation. This is because the code for selecting a child resource by name given the parent is generated in the `lookup_resource!` invocation for the *parent* resource, which doesn't *know* whether the child has `lookup_by_name = true` or `lookup_by_name = false`. This means that this code is unconditionally generated, even when there is no name column for the child (i.e., it is an `Asset` rather than a `Resource`). While we currently have plenty of `lookup_resource!` invocations for `Asset`s, none of them are currently the child of another resource. While working on the webhook implementation, I wanted the `WebhookSecret` resource to be a child of the `WebhookReceiver` resource for authz purposes, but secrets do not have names. This resulted in the `lookup_resource!` macro generating code that wouldn't compile. This commit fixes that issue by changing the child selector functions from being generated by the parent's `lookup_resource!` invocation to instead being generated by the child's `lookup_resource!` invocation. This way, we can decide whether or not to generate the child-by-name selectors based on whether or not the child has `lookup_by_name = true` or not. Since all the `lookup_resource!` invocations are in the same file, it's alright for the child's `lookup_resource!` invocation to generate `impl` blocks for the parent as well.
nexus/db-macros/outputs/project.txt
Outdated
There was a problem hiding this comment.
The diff here is anticipated based on this change and shouldn't actually cause any problems. Because the test only generates code for Project and not for any other types, the methods that are generated have changed: now, the child-by-name accessors on Project for Disk and Instance are no longer generated, but the child-by-name accessor on Silo for Project is generated. This shouldn't actually be a problem in real life, as we will always invoke the lookup_resource! macro for both the parent and child types, so all the impls should still be generated. Note that Nexus still compiles after this change :)
| // N.B. that this is no longer used by the the parent's resource codegen, | ||
| // but it may be again in future. Therefore, continue parsing it so that | ||
| // the resource definitions don't change, but allow it to be unused. | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
FWIW I'll defer to @davepacheco here but I wouldn't be opposed to removing this as an input.
(We could always re-add it later if necessary, but it seems nice to only need to specify the parent, not the parent + children)
There was a problem hiding this comment.
Mmm, I'd also be fine with removing it. My thinking was that, presently, the resource-definition file has lists of children in it, and we could remove that, but if we wanted to later add codegen to the parents that depended on knowing all the children, we'd have to add that information again. I felt like that could potentially be a minor hassle: if we add more child resources, we wouldn't be able to just check out the code from before the lists were removed, and we'd instead have to manually reconstruct them. But...maybe that isn't actually a big deal, IDK.
| if !config.lookup_by_name { | ||
| return quote! {}; | ||
| } |
There was a problem hiding this comment.
To confirm: This is like, the "functional crux" of the change, the rest is moving the generation from "generated in parent declaration" to "generated in child declaration", which is more of a refactor.
There was a problem hiding this comment.
Yeah, that's correct. Skipping this generation when the child resource can only be looked up by ID is necessary to stop generating code that won't compile for such resources. Moving around where that code gets generated is only necessary because otherwise, we wouldn't have the information about whether the child is looked up by name.
Co-authored-by: Sean Klein <sean@oxide.computer>
Presently, the
lookup_resource!macro generates code that will not compile when a child resource haslookup_by_name = falsein itslookup_resource!macro invocation.This is because the code for selecting a child resource by name given the parent is generated in the
lookup_resource!invocation for the parent resource, which doesn't know whether the child haslookup_by_name = trueorlookup_by_name = false. This means that this code is unconditionally generated, even when there is no name column for the child (i.e., it is anAssetrather than aResource).While we currently have plenty of
lookup_resource!invocations forAssets, none of them are currently the child of another resource. While working on the webhook implementation, I wanted theWebhookSecretresource to be a child of theWebhookReceiverresource for authz purposes, but secrets do not have names. This resulted in thelookup_resource!macro generating code that wouldn't compile. I've cherry-picked this commit from that branch to try and land it separately.This commit fixes that issue by changing the child selector functions from being generated by the parent's
lookup_resource!invocation to instead being generated by the child'slookup_resource!invocation. This way, we can decide whether or not to generate the child-by-name selectors based on whether or not the child haslookup_by_name = trueor not. Since all thelookup_resource!invocations are in the same file, it's alright for the child'slookup_resource!invocation to generateimplblocks for the parent as well.