Update lookup fns introduced in the V1 API to return lookup::* over authz::*

[just-be-dev]

This PR is mostly in response to @davepacheco's feedback on #1957. Special thanks to @smklein for helping work through some rust lifetime issues here.

Previously the instance_lookup and project_lookup functions returned a authz::Instance or authz::Project respectively that was authorized with read permissions. The challenge here is that this meant that in several places we required duplicate lookups to be able to resolve the instance with the right permissions. To avoid that we're passing around the lookup step itself instead.

[david-crespo]

Awesome. It feels very clearly like the way it's meant to be done. We've been working our way here for at least a year!

[augustuswm]

Overall this makes sense to me. I don't know enough about Nexus to know if this would work or fit with existing patterns, but following this pattern we are likely to end up with a lot of match cases for project scoped resources that mirror:

ResourceSelector {
    resource: NameOrId::Name(resource_name),
    project: Some(NameOrId::Name(project_name)),
    organization: Some(NameOrId::Id(organization_id)),
} => {
    // ...
}
ResourceSelector {
    resource: NameOrId::Name(resource_name),
    project: Some(NameOrId::Name(project_name)),
    organization: Some(NameOrId::Name(organization_name)),
} => {
    // ...
}

Outside the scope of this PR, but maybe InstanceSelector could look like:

struct InstanceSelector {
    pub instance: NameOrId,
    pub project: Option<ProjectSelector>,
}

which would reduce the needed branches per selector. Following this path though would unfortunately start replicating a lot of the logic already encoded in the the db::lookup enums.

This shouldn't block this PR, but from my limited reading, it seems like with Selectors we are starting to encode the hierarchy of resources in multiple places.

[just-be-dev]

The selectors have a 1-to-1 relationship with query params so we can't really nest selectors like that. Each selector has to represent all possible query parameters. That said, we might be able to reduce the length of the selectors to lookup match statements.

[augustuswm]

Right, I should have been clearer (and used a different name) on that. It would end up as a separate struct, such that the chain became Params -> Selector -> Lookup

[davepacheco]

Yeah, @augustuswm's point makes a lot of sense to me. I had been assuming that if we had these functions return lookup::Foo types then they could compose with each other (i.e., instance_lookup() would just use project_lookup() directly). I see that doesn't quite work unless we make InstanceSelector contain (or at least have a way to get) a ProjectSelector. Right now it seems like there's still a fair bit of code duplication in these lookup functions, that's replicated for every resource, and it only gets worse as you go down the hierarchy. I thought about suggesting that instance_lookup() accept a lookup::Project as an argument, but that doesn't make sense in the by-id case. You really need to create the lookup::Project only if what you got was an instance name.

I can see how this could work if we impl From<InstanceSelector> for ProjectSelector and then have instance_lookup() match only on the instance part, and in the "name" branch, turn the InstanceSelector into a ProjectSelector and use that to call project_lookup(). The downside is we have to impl all those Froms. But I think it would be an improvement.

And then yeah as @augustuswm mentioned, I wonder if there's a way to express the input types so that we don't have to write all those conversions. What if InstanceSelector contained a ProjectSelector via #[serde(flatten)]?

[davepacheco]

Yeah, so maybe this should require Modify.

[just-be-dev]

I agree generally with the feedback here. I'm going to merge this PR and do a few more endpoints (definitely those lower in the tree like project and org). After those are all in, I'll start investigating scoping things down for composability.