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.

Sign up for GitHub

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

[db-macros] support for children without names in lookup_resource! #7508

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[db-macros] support for children without names in lookup_resource! #7508

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e3f6307
[db-macros] support for children without names in `lookup_resource!`
hawkw Feb 7, 2025
bd3edec
fix dead code warning
hawkw Feb 7, 2025
c93b0bd
regen test snapshots
hawkw Feb 10, 2025
9dfa35b
@smklein's comment suggestion
hawkw Feb 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 17 additions & 30 deletions nexus/db-macros/outputs/project.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,36 +15,6 @@ pub enum Project<'a> {
PrimaryKey(Root<'a>, Uuid),
}
impl<'a> Project<'a> {
///Select a resource of type Disk within this Project, identified by its name
pub fn disk_name<'b, 'c>(self, name: &'b Name) -> Disk<'c>
where
'a: 'c,
'b: 'c,
{
Disk::Name(self, name)
}
///Select a resource of type Disk within this Project, identified by its name
pub fn disk_name_owned<'c>(self, name: Name) -> Disk<'c>
where
'a: 'c,
{
Disk::OwnedName(self, name)
}
///Select a resource of type Instance within this Project, identified by its name
pub fn instance_name<'b, 'c>(self, name: &'b Name) -> Instance<'c>
where
'a: 'c,
'b: 'c,
{
Instance::Name(self, name)
}
///Select a resource of type Instance within this Project, identified by its name
pub fn instance_name_owned<'c>(self, name: Name) -> Instance<'c>
where
'a: 'c,
{
Instance::OwnedName(self, name)
}
/// Fetch the record corresponding to the selected resource
///
/// This is equivalent to `fetch_for(authz::Action::Read)`.
Expand Down Expand Up @@ -393,3 +363,20 @@ impl<'a> Project<'a> {
Ok((authz_silo, authz_project, db_row))
}
}
impl<'a> Silo<'a> {
///Select a resource of type Project within this Silo, identified by its name
pub fn project_name<'b, 'c>(self, name: &'b Name) -> Project<'c>
where
'a: 'c,
'b: 'c,
{
Project::Name(self, name)
}
///Select a resource of type Project within this Silo, identified by its name
pub fn project_name_owned<'c>(self, name: Name) -> Project<'c>
where
'a: 'c,
{
Project::OwnedName(self, name)
}
}
100 changes: 52 additions & 48 deletions nexus/db-macros/src/lookup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ pub struct Input {
ancestors: Vec<String>,
/// unordered list of resources that are direct children of this resource
/// (e.g., for a Project, these would include "Instance" and "Disk")
// 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)]
Comment on lines +39 to +42
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove this if we're ignoring it. We can always go back in the Git history, but in the meantime, having it here but ignored seems confusing to readers (especially if the values supplied wind up drifting from what would be correct, which seems likely).

Is this the only thing children was used for? (If so, why would it not work to leave out WebhookSecret from the list of children of WebhookReceiver?)

If we can get away with this, it does seem better. I imagine I just didn't think of it when I first did this work. It's always been less than ideal that you had to specify both parent and children!

children: Vec<String>,
/// whether lookup by name is supported (usually within the parent collection)
lookup_by_name: bool,
Expand Down Expand Up @@ -127,11 +131,6 @@ pub struct Config {
/// [`authz_silo`, `authz_project`])
path_authz_names: Vec<syn::Ident>,

// Child resources
/// list of names of child resources, in the same form and with the same
/// assumptions as [`Input::name`] (i.e., typically PascalCase)
child_resources: Vec<String>,

// Parent resource, if any
/// Information about the parent resource, if any
parent: Option<Resource>,
Expand Down Expand Up @@ -160,7 +159,6 @@ impl Config {
.collect();
path_authz_names.push(resource.authz_name.clone());

let child_resources = input.children;
let parent = input.ancestors.last().map(|s| Resource::for_name(s));
let silo_restricted = !input.visible_outside_silo
&& input.ancestors.iter().any(|s| s == "Silo");
Expand All @@ -177,7 +175,6 @@ impl Config {
path_types,
path_authz_names,
parent,
child_resources,
lookup_by_name: input.lookup_by_name,
primary_key_columns,
soft_deletes: input.soft_deletes,
Expand Down Expand Up @@ -221,22 +218,22 @@ pub fn lookup_resource(
let resource_name = &config.resource.name;
let the_basics = generate_struct(&config);
let misc_helpers = generate_misc_helpers(&config);
let child_selectors = generate_child_selectors(&config);
let child_selector = generate_child_selector(&config);
let lookup_methods = generate_lookup_methods(&config);
let database_functions = generate_database_functions(&config);

Ok(quote! {
#the_basics

impl<'a> #resource_name<'a> {
#child_selectors

#lookup_methods

#misc_helpers

#database_functions
}

#child_selector
})
}

Expand Down Expand Up @@ -287,63 +284,70 @@ fn generate_struct(config: &Config) -> TokenStream {
}
}

/// Generates the child selectors for this resource
/// Generates the child selector for this resource's parent
///
/// For example, for the "Project" resource with child resources "Instance" and
/// "Disk", this will generate the `Project::instance_name()` and
/// `Project::disk_name()` functions.
fn generate_child_selectors(config: &Config) -> TokenStream {
let child_resource_types: Vec<_> =
config.child_resources.iter().map(|c| format_ident!("{}", c)).collect();
let child_selector_fn_names: Vec<_> = config
.child_resources
.iter()
.map(|c| format_ident!("{}_name", heck::AsSnakeCase(c).to_string()))
.collect();
let child_selector_fn_names_owned: Vec<_> = config
.child_resources
.iter()
.map(|c| {
format_ident!("{}_name_owned", heck::AsSnakeCase(c).to_string())
})
.collect();
let child_selector_fn_docs: Vec<_> = config
.child_resources
.iter()
.map(|child| {
format!(
"Select a resource of type {} within this {}, \
identified by its name",
child, config.resource.name,
)
})
.collect();
/// For example, for the "Instance" resource with parent resource "Project",
/// this will generate the `Project::instance_name()` and
/// `Project::instance_name_owned()` functions.
///
/// This is generated by the child resource codegen, rather than by the parent
/// resource codegen, since such functions are only generated for resources with
/// `lookup_by_name = true`. Whether or not this is enabled for the child
/// resource is not known when generating the parent resource code, so it's
/// performed by the child instead.
fn generate_child_selector(config: &Config) -> TokenStream {
// If this resource cannot be looked up by name, we don't need to generate
// child selectors on the parent resource.
if !config.lookup_by_name {
return quote! {};
}
Comment on lines +301 to +303
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// The child selector is generated for the parent resource type. If there
// isn't one, nothing to do here.
let Some(ref parent) = config.parent else { return quote! {} };

let parent_resource_type = &parent.name;
let child_selector_fn_name = format_ident!(
"{}_name",
heck::AsSnakeCase(config.resource.name.to_string()).to_string()
);

let child_selector_fn_name_owned = format_ident!(
"{}_name_owned",
heck::AsSnakeCase(config.resource.name.to_string()).to_string()
);
let child_resource_type = &config.resource.name;

let child_selector_fn_docs = format!(
"Select a resource of type {child_resource_type} within this \
{parent_resource_type}, identified by its name",
);

quote! {
#(
impl<'a> #parent_resource_type<'a> {
#[doc = #child_selector_fn_docs]
pub fn #child_selector_fn_names<'b, 'c>(
pub fn #child_selector_fn_name<'b, 'c>(
self,
name: &'b Name
) -> #child_resource_types<'c>
) -> #child_resource_type<'c>
where
'a: 'c,
'b: 'c,
{
#child_resource_types::Name(self, name)
#child_resource_type::Name(self, name)
}

#[doc = #child_selector_fn_docs]
pub fn #child_selector_fn_names_owned<'c>(
pub fn #child_selector_fn_name_owned<'c>(
self,
name: Name,
) -> #child_resource_types<'c>
) -> #child_resource_type<'c>
where
'a: 'c,
{
#child_resource_types::OwnedName(self, name)
#child_resource_type::OwnedName(self, name)
}
)*
}
}
}

Expand Down
Loading