Ephemeral mappers#449
Conversation
notpeter
left a comment
There was a problem hiding this comment.
At first glance this looks fine.
-
As I mentioned in the other review I don't love the
Ephemeralterm because they can't go away. They are perhaps static, stateless, immutable but ephemeral things are short lived and these are largely immortal. Open to other terms. -
I think this could use a db transaction. Currently
record_mapper_eventis triggered beforeregister_api_user(which could fail). Ideally these would occur in the same transaction so if the user create failed we don't have a mapper_event that didn't actually happen. Not sure how difficult that is to thread, but wanted to mention it. -
No referential integrity for mapper_event.user_id -- often times audit tables intentionally don't have foreign keys (for good reasons) but I think they might be reasonable here.
|
|
||
| // Convert ephemeral mapper configs into Mapper structs with deterministic IDs | ||
| let ephemeral_mappers: Vec<Mapper> = self | ||
| .mappers |
There was a problem hiding this comment.
- Should we validate each rule? (e.g.
mapping_ctx.validate(&config.rule)) - Can we catch duplicate names? Since the UUID is deterministic based on name a copy pasta error in config could be wonky.
There was a problem hiding this comment.
- Yes, absolutely this should be validating. I'll fix that.
- Yeah, easy enough to verify if we have duplicate names are fail out early here.
| @@ -0,0 +1 @@ | |||
| DROP TABLE IF EXISTS mapper_event; | |||
There was a problem hiding this comment.
Year is wrong on these migrations.
| pub mapper_name: String, | ||
| pub user_id: TypedUuid<UserId>, | ||
| pub rule: Value, | ||
| pub ephemeral: bool, |
There was a problem hiding this comment.
nit: Should MapperEvent.ephemeral and NewMapperEvent.ephemeral actually be MapperSource?
There was a problem hiding this comment.
Yeah it should. That way we can expand the variants if needed.
| match self.consume_mapping_activation(&mapper).await { | ||
| Ok(_) => true, | ||
| Err(err) => { | ||
| // TODO: Inspect the error. We expect to see a conflict error, and |
There was a problem hiding this comment.
Is this comment outdated? I would expect the upsert in consume_mapping_activation wouldn't result in conflicts.
|
Adds the concept of ephemeral mappers. Ephemeral mappers are mappers that exist only within the runtime memory of the server. Their purpose is to solve a pain point around defining mappers by configuration as opposed to database state.
Main differences:
Generally an application will require at least one mapper rule to bootstrap itself. This is something like an admin group with a user that should be the initial admin. From here, that admin can create groups, and additional dynamic mappers that are stored alongside the data of the application. This bootstrapping mapper is a good use case for an ephemeral mapper. The user defines a configuration for an ephemeral mapper that is passed to the v-api context builder at runtime, and that mapper is installed in memory. An admin can then log in and be granted permissions. On restart, the user can decide if they still want to pass this configuration to the v-api context or not. If they don't then the mapper rule acts as if it never existed.
Because these mappers are ephemeral, we also need to include the ability to record when they are processed. This change adds a log table that records the execution results of mappers so that independent of a mapper being ephemeral or dynamic, we still know when it was used to grant permissions