Conversation
cb796df to
ac9b544
Compare
| /// Reusable filter input types for GraphQL queries. | ||
|
|
||
| #[derive(Debug, Clone, Default, async_graphql::InputObject)] | ||
| pub struct BoolFilter { |
There was a problem hiding this comment.
Wanting to start a parallel discussion here about standardizing optional filters that could eventually be exposed in the UI. Bool is the simplest example, here are a couple others that would follow the pattern:
#[derive(Debug, Clone, Default, async_graphql::InputObject)]
pub struct StringFilter {
pub eq: Option<String>,
pub ne: Option<String>,
pub starts_with: Option<String>,
pub contains: Option<String>,
}
#[derive(Debug, Clone, Default, async_graphql::InputObject)]
pub struct IntFilter {
pub eq: Option<i64>,
pub ne: Option<i64>,
pub gt: Option<i64>,
pub gte: Option<i64>,
pub lt: Option<i64>,
pub lte: Option<i64>,
}There was a problem hiding this comment.
Like I was talking about below, unless we're applying these filters over a homogenous-enough surface area (i.e, it's not actually clear to me that the UI should be the same datagrid/table sort of thing for captures, collections, materializations, storage mappings, data planes, tenant members, etc), then I'd actually vote for keeping it specific to each resolver. Thoughts?
Or maybe keep these StringFilter/IntFilter reusable chunks, but only implement them as needed in the resolvers where they make sense?
There was a problem hiding this comment.
i think this is the right compromise - having a standardized pattern can only help, and how far we take the implementation depends on what's needed vs how painful it gets as it grows
There was a problem hiding this comment.
I can definitely see the benefit of having consistent terminology for different predicates. Though a problem I foresee with the approach is that not all resolvers will be able to support all predicates. For example, for some we might only support eq, and others maybe only startsWith. I mostly just wouldn't want to have our graphql schema suggest that you could use a contains filter on liveSpecs names, for example, when we don't want to support such queries in the database.
I'm a little on the fence about liveSpecs(by: { prefix: "acmeCo/" }) vs liveSpecs(by: { catalogName: { startsWith: "acmeCo/" } }). I do kinda like the latter, though IDK whether I'd like it better enough to justify migrating existing resolvers or living with two different conventions. Maybe it'd help to understand what the migration process would look like? If we wanted to have some limited backward compatibility during a transition period, would that be pretty easy?
There was a problem hiding this comment.
migration would be easy - we can deprecate the current param and add it to the filter section ... decide later when we think it's safe to remove the deprecated one.
| /// The capability level granted by this invite link. | ||
| pub capability: models::Capability, | ||
| /// Whether this invite link can only be used once. | ||
| pub single_use: bool, |
There was a problem hiding this comment.
this replaces the directives.uses_remaining field
(and we'll delete consumed invites, instead of setting uses_remaining = 0)
There was a problem hiding this comment.
test comment please ignore
| /// The secret token for this invite link. | ||
| pub token: uuid::Uuid, | ||
| /// The catalog prefix this invite link grants access to. | ||
| pub catalog_prefix: models::Prefix, |
There was a problem hiding this comment.
related to the optional filters, i'd like these resolvers to do their own check of which prefixes the user can see, and then this catalog_prefix can be an optional filter to narrow the results further.
would love to talk this through w/ phil & joseph
There was a problem hiding this comment.
The other thing i like about this is that we can define the operators and standardize the prefix filters that phil and i were chatting about a few weeks ago (prefix vs prefixedBy vs exactPrefixes` etc...)
#[derive(Debug, Clone, Default, async_graphql::InputObject)]
pub struct PrefixFilter {
pub eq: Option<models::Prefix>,
pub starts_with: Option<models::Prefix>,
}so an example gql query might look like
query links {
inviteLinks(filter: {
singleUse: {
eq: true
}
catalogPrefix: {
startsWith: "gregCo"
}
}) {
...
}
}There was a problem hiding this comment.
Yeah... I both do and don't like this. Back when I wrote a very similar generalized graphql resolver filter for Narwhal's GraphQL API, the reason it was tractable at all was that the entities we were querying over were homogenous enough that there was one centralized place (the "query builder") where we could turn these filters (startsWith, lt, eq, and, or, etc) into a big WHERE clause which could then be applied to any query we wanted.
If we can't do that, then it becomes a combinatorial nightmare of having to support every type of filter on every graphql resolver's individual query/queries.
There was a problem hiding this comment.
I will also note, I spent a lot of time in there optimizing the monstrous queries it would generate. So we should probably not follow that same exact pattern here :P
24d51c2 to
e1271ed
Compare
| created_at AS "created_at!: chrono::DateTime<chrono::Utc>" | ||
| FROM internal.invite_links | ||
| WHERE catalog_prefix::text ^@ $1 | ||
| AND ($4::bool IS NULL OR single_use = $4) |
There was a problem hiding this comment.
optional filters will make these queries fairly long...
|
|
||
| // Delete single-use invite links upon redemption. | ||
| if invite.single_use { | ||
| sqlx::query!("DELETE FROM internal.invite_links WHERE token = $1", token,) |
There was a problem hiding this comment.
just calling out new behavior - delete single-use invites when they're redeemed
| tracing::info!( | ||
| %catalog_prefix, | ||
| ?capability, | ||
| %claims.sub, |
There was a problem hiding this comment.
@jshearer i suppose this could be the record of who invited the user...
There was a problem hiding this comment.
We don't keep logs indefinitely. If it's not required for compliance then I'm 100% whatevs about tracking who invited whom, I was just spitballing features that the existing directive's implementation could offer that we might care about
242ab53 to
8904603
Compare
5bcf582 to
56a525c
Compare
psFried
left a comment
There was a problem hiding this comment.
I found a few things that are probably worth changing. I'm still interested in resolving the conversation on BoolFilter, but just wanted to get you the feedback that I have now.
| cursor: String! | ||
| } | ||
|
|
||
| input InviteLinksFilter { |
There was a problem hiding this comment.
I actually like this _Filter naming better, though the inconsistency with _By seems unfortunate. IMO if we're going to switch the convention, then it's probably worth trying to switch everything else over, too. I'm kinda on the fence as to whether it's worth while, though. What do you think?
| /// Reusable filter input types for GraphQL queries. | ||
|
|
||
| #[derive(Debug, Clone, Default, async_graphql::InputObject)] | ||
| pub struct BoolFilter { |
There was a problem hiding this comment.
I can definitely see the benefit of having consistent terminology for different predicates. Though a problem I foresee with the approach is that not all resolvers will be able to support all predicates. For example, for some we might only support eq, and others maybe only startsWith. I mostly just wouldn't want to have our graphql schema suggest that you could use a contains filter on liveSpecs names, for example, when we don't want to support such queries in the database.
I'm a little on the fence about liveSpecs(by: { prefix: "acmeCo/" }) vs liveSpecs(by: { catalogName: { startsWith: "acmeCo/" } }). I do kinda like the latter, though IDK whether I'd like it better enough to justify migrating existing resolvers or living with two different conventions. Maybe it'd help to understand what the migration process would look like? If we wanted to have some limited backward compatibility during a transition period, would that be pretty easy?
Also including transitional dual-write triggers and backfill migration so we can move off of the directive-based invite links. These will be removed in a future migration.
56a525c to
8ecfa79
Compare
internal.invite_linkstable to replace the directives-based invite flow with a simplified modelinviteLinks,createInviteLink,redeemInviteLink,deleteInviteLink)public.directivesandinternal.invite_links, plus a backfill of existing unredeemed grant directives — to be removed after the UI adopts the new GraphQL APITest plan
internal.invite_links