fix(db): reject application models in migrations - #625
Conversation
github-actions
Bot
added
C-lib
| #[derive(Debug, Clone)] | ||
| pub struct Database { | ||
| inner: Arc<DatabaseImpl>, | ||
| migration_context: bool, |
There was a problem hiding this comment.
Let's use enum here instead of bool. If we have something like enum DatabaseContext { Default, InMigration }, a match will be a bit easier to understand in the places where we use this field, and will also allow us to extend the set of possible contexts, if ever needed.
| const COLUMNS: &'static [Column]; | ||
|
|
||
| #[doc(hidden)] | ||
| const IS_APPLICATION_MODEL: bool = true; |
There was a problem hiding this comment.
I don't think this should be #[doc(hidden)]; it's fine to expose this information. Instead of a bool, however, I think we should define a new public enum, mirroring the one we currently have in cot-codegen:
#[non_exhaustive]
pub enum ModelType {
Application,
Migration,
Internal,
}
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Updated both. Database now uses DatabaseContext, and models expose a public non-exhaustive ModelType. The Cot and macro test suites and clippy pass. |
| } | ||
|
|
||
| fn ensure_model_allowed<T: Model>(&self) -> Result<()> { | ||
| match (self.context, T::MODEL_TYPE) { |
There was a problem hiding this comment.
Shouldn't we handle (DatabaseContext::Default, ModelType::Migration) here as well? I can't come up with a case where it makes sense 🤔
3 participants
Fixes #623. Custom migrations now reject application models before they can access the database. Migration models and raw SQL keep working. Tested with the full cot suite, the macro suite, and clippy.