This kind of strange change makes it possible to use bail_unless! in a situation where your function isn't returning Error per se, but something that impls From<Error>.

I found the combination of ApiResource and ApiResourceError unnecessarily confusing, so I combined them. This basically meant removing the Clone + oso::PolarClass trait bounds here (so that ApiResource can be object-safe) and adding them back (actually oso::ToPolar) in the few places where we actually need them.

I don't like the thought that every resource will have its own copy of these three types, though if the roles can be different I guess we have no choice?

Yeah, I think this is a tradeoff for having the allowed roles be expressed in the API. If there's another way to do it without that tradeoff that'd be great!

Will all such types be the same except for their names?

If they will, then one approach would be to impl Into<RolesPolicy> for OrganizationRolesPolicy on all the individual types. That would give them all the same type in the OpenAPI spec.

But I see what we're buying with the separate types: a generated client will already know at the type level what roles are valid for a given resource, which is pretty nifty.

type OrganizationRoles = 'Admin' | 'Collaborator'

type OrganizationRolesPolicy = {
  roleAssignments: OrganizationRolesRoleAssignment[]
}

type OrganizationRolesRoleAssignment = {
  identityId: string
  identityType: IdentityType
  roleName: OrganizationRoles
}

I guess I'm basically lamenting that OpenAPI doesn't let us express this relationship (as far as I know):

type RolesPolicy<Role extends string> = {
  roleAssignments: RoleAssignment<Role>[]
}

type RoleAssignment<Role extends string> = {
  identityId: string
  identityType: IdentityType
  roleName: Role
}

type OrganizationRole = 'Admin' | 'Collaborator'
// use RoleAssignment<OrganizationRole> wherever

type ProjectRole = 'Admin' | 'Collaborator' | 'SomethingElse'
// use RoleAssignment<ProjectRole> wherever

Yeah, it would be neat if OpenAPI let you do that.

To summarize: my primary intent here was to have the Nexus type reflect the set of allowed role names. That way, this validation is handled like all other validation, while parsing the input in the first place, and the other code paths can know from the type that the role names have been validated.

I went the logical next step of using this type directly in the HTTP handler functions (both input and output) so that the OpenAPI spec reflects the allowed values, especially since we do validate that so we may as well document what the accepted values are in the spec. As you noted, this means the generated clients can know exactly which values are allowed, which is cool. We don't have to do this part though. We could still accept and return strings instead of enums. And it's possible that in the future we'll want to do that because if we support custom roles, then there won't be an enum of allowed values. But I'd rather not spend much time now worrying about that for the usual reasons: it's hard to know we're actually solving the future problem no matter what we do now, and we can always evolve this API compatibly anyway.

@david-crespo is your lament, in essence, that OpenAPI / JSON Schema doesn't permit the expression of generics types / types with parameters?

Yes — having identical *RolesPolicy and *RoleAssignment types for every resource feels silly. But it seems there's no way around it if we want to keep the per-resource role enums as opposed to making them all string (or make them all share the same enum).

I see that there is dissensus about how we name enum variants. I'm not sure how we started doing kebab case, but I think that other things are doing snake_case.

1. it obviously doesn't matter with these variants
2. it's internal so who cares (but we do have both in the external API)
3. I think we should probably add this to the linter (and be "we" I mean "I" and by "should" I mean "what do you think?")

Agreed. I don't think I care between snake_case and kebab-case. I'm sure I copied this from somewhere else thinking it was our standard. Yes, it'd be great to lint it!

I think that we can now do express this with Dropshot if that's what you mean... which is not to say this should be done now, but are there pieces missing to make this tenable?

I suspect there's a lot of work here but I don't know. I feel like several times over the last year I've thought "ugh, <thing I'm working on> is gonna get complicated when we implement etags". But thinking out loud:

• We need to decide how to generate the etag for every kind of resource. Is it random (and updated with every modification) or content-based? If content-based, is it stored in the database or calculated when needed? If calculated when needed, it'd be nice if we could do it in SQL so that you can calculate it and check it within a SQL query (see below). What fields go into it? How do we specify that on a per-resource basis? How do we keep it up to date if you're doing something like update ... set ... where <condition that matches 10 rows>?
• We need to plumb it through on "list" and "get" for everything. Gotta figure out where to store it...if it's stored in
  the database, great. If not, how do we pass it around? Maybe it goes into the authz object? That might sound weird but the authz object basically represents a handle to a specific object, regardless of how it was looked up or what it's called now. It makes sense that would also be locked to a specific revision of that object.
• We need to modify all the "update" datastore functions so that they accept an optional etag and do the conditional update. In an ideal world, we'd issue a SQL "update" with the etag condition in it (rather than read-check-write).
• We need to plumb that optional etag argument out to all the PUT entrypoints.

That doesn't sound so bad. We might need better transaction support (see #973) -- I'm not sure.

At the time we split out so-called views and params, I think we anticipated the need for types that might exist in both. Is there a reason to maintain that split? The current files enforce an organization that doesn't particularly make sense to me. We could, for example, have groups of endpoints and types live together. ... Obviously not a blocker for this review...

I agree that the current split is confusing. I understand the current guidance to be:

• if it's an input param, it goes in "params"
• if it's an output, it goes in "views"
• if it's both, it goes in "shared"
• there's also a bunch of stuff in "common" that hasn't been put into one of those three places because it's hard

This last point makes it kind of a mess, and it's also possible that this last part is hard because there are other constraints involved that this guidance doesn't accommodate. Progenitor changes a bunch of this too -- I think part of the original reason for "common" was for stuff that was shared between clients and servers of a given service.

On your specific example:

We could, for example, have groups of endpoints and types live together.

We could. Any way I can think to group them sucks a little bit -- including the current approach of putting them in one big file. (If we group them by resource, one might be sad that the various "create" endpoints (which are very parallel to each other and use some of the same types, like IdentityMetadataCreate) don't live near each other. Also, it's kind of nice for some small groups of resources to live together (e.g., built-in users and roles, which might go under an "IAM" bucket). So maybe this is by tag after all?)

Basically, I don't have a strong idea of how these should be better organized.

Forgive me as this is neither here nor there, but the docs here become the external docs. I think we'll eventually want to change this to be something like:

💯 agree -- and I think that'll be true for all the stuff in views. (I copied the form there for these types.) I think we'll want to make an edit pass that documents all these things properly.

I wasn't sure where this came from, but this seems a bit circular in terms of customer-facing docs

@david-crespo is your lament, in essence, that OpenAPI / JSON Schema doesn't permit the expression of generics types / types with parameters?