Remove static models by yannik131 · Pull Request #1551 · biigle/core

yannik131 · 2026-08-25T19:06:56Z

Closes #1048

yannik131 · 2026-08-27T15:46:10Z

Migration guide for me

Turning eloquent model X into enum X requires these changes:

Enum implementation

Assuming all possible model values are written into the corresponding X table upon migration

Find values by connecting to the db (docker compose exec database psql -U biigle -d biigle -c "select * from X;") and by looking at DB::table('X')->insert statements, insertion order corresponds to values, 1-indexed
Assign integer values to each existing case using the same values and implement
cId() returning the value of case c for each case
c() returning the case cfor each case
label() returning the case name like it was previously written in the DB
toArray() returning ['id' => xId(), 'name' => label()]

Creating the migration

Find all foreign keys and their constraint names using docker compose exec database psql -U biigle -d biigle -c "\d+ X", then

(make sure enum and database table are 1:1 congruent on present IDs/values as a failsafe: MAYBE if migrations don't fill the tables completely?)
drop all fk constraints
drop the table itself

Editing old migrations

Instead of removing the tables from the migrations, it's easier to keep them and replace eloquent model method calls X::method() (all(), insert(), ...) with DB::table('X')->method().

Common needed replacements

X is the name of the model and enum, x is lowercase name, x_id is (usually) the name of the FK

X::all() -> collect(X::cases())->map->toArray() (add ->values() if PHPStan is pedantic)
$val = X::findOrFail($caseId) -> $val = X::tryFrom((int) $caseId); abort_if($val === null, 404);
similarly, other model-specific query patterns X::find(), whereHas(), with(), whereRelation()
collect([X::case1(), X::case2(), ..])-> collect(...)->map->toArray()
FormRequest validation rules like 'x_id' => 'integer|exists:x,id' -> 'x_id' => ['integer', Rule::in(array_column(X::cases(), 'value'))]
$this->belongsTo(X::class) -> X::from($this->x_id)
Eloquent model has x() method -> getXAttribute(): X. This way $object->x still works
blade files: {!! Biigle\X::x() !!} -> @json(Biigle\X::x()->toArray())
$x->id -> $x->value
$x->name -> $x->label()
$object->x()->associate(Biigle\X::case()) -> $object->x_id = X::caseId()
remove DB-specific X-tests
XTest::create() -> X::case() (find out default case or just choose one)
X::pluck('id', 'name') -> collect(X::cases())->mapWithKeys(fn (X $x) => [$x->label() => $x->value])
Delete factory XFactory, replace X::factory() calls with some default X::case1Id()
Remove eager loading statements ->with('x') from DB queries

Will do that once the PR has been reviewed

yannik131 · 2026-08-31T17:06:37Z

@mzur I've started working on the maia code, but this PR is ready for review. It shouldn't be merged before the maia PR though. Most of the changes are the result of the above migration guide I created for myself. There are a few TODOs scattered through the code, but they could maybe just be removed. And I can't really test the entirety of the application, but the tests pass and the basic things I tried locally (annotations, reports, admin area, ...) worked without issues. I've long been thinking about creating an E2E regression test with something like Cypress for regression cases like this, what is your opinion on that? Just a single test that logs in and tests some basic things (create a project, open image/video annotation tool, create an annotation, generate a report, ...).
I would like to put the enums into a separate Enums/ directory. I just didn't do it yet because it would affect hundreds of files and make the PR difficult to review, so I'll do it last when everything else is settled.

mzur

I'm thinking if we switch to this new pattern we could just as well embrace it completely although it may be a lot of work. On the other hand, this could be a lot of work with only marginal performance gains (the performance impact of cached models could be minimal although it saves network roundtrips to the cache). It could be mostly a logical refactor without tangible gains...

Since you already put some work into this: Do you feel the change is worth it? We could still pull the plug before putting even more work into this.

If we go ahead:

I think the cleanest approach would be to rename columns like volumes.media_type_id to volumes.media_type and then use enum casting for this property. This way we don't have to implement custom accessors for every enum property.

Instead of implementing the enums to mimic the old static models (with MediaType::video() or MediaType::videoId() methods, for example) I'd fully switch to enum syntax (i.e. MediaType::VIDEO and MediaType::VIDEO->value). This would require updates to every place a static model was used (including modules).

Let's discuss the high-level comments first. I didn't go into too much detail below.

mzur · 2026-09-02T13:37:40Z

            ],
            'confidence'  => 'required|numeric|between:0,1',
-            'shape_id' => 'required|integer|exists:shapes,id',
+            'shape_id' => ['required', 'integer', Rule::in(Shape::pluckById()->keys()->all())],


Isn't this equivalent to:
(also for other validation cases)

Suggested change

'shape_id' => ['required', 'integer', Rule::in(Shape::pluckById()->keys()->all())],

'shape_id' => ['required', 'integer', Rule::enum(Shape::class)],

mzur · 2026-09-02T13:39:31Z

    {
        // Image annotations cannot have the whole frame shape.
-        $shapeIds = Shape::whereKeyNot(Shape::wholeFrameId())->pluck('id');
+        $shapeIds = Shape::pluckById(Shape::wholeFrame())->keys();


A little confusing syntax. Maybe use:

Suggested change

$shapeIds = Shape::pluckById(Shape::wholeFrame())->keys();

$shapeIds = Shape::pluckById(except: Shape::wholeFrame())->keys();

mzur · 2026-09-02T13:51:33Z

+    public function toArray(): array
+    {
+        return [
+            'id' => $this->value,
+            'name' => $this->label()
+        ];
+    }
+
+    #[Override]
+    public function jsonSerialize(): mixed
+    {
+        return $this->toArray();
+    }


Maybe put this into a trait (or Laravel already has something like this)?

mzur · 2026-09-02T13:53:07Z

+    case IMAGE = 3;
+    case VIDEO = 4;


You can't rely on these IDs in the DB. The migration must override the existing IDs in the DB to match the enums in all cases. The enums can have new IDs starting at 1.

I was wondering about that. Given the migrations execute changes in a defined order and that insert statements result in 1-indexed IDs also with a defined order I assumed the DB-values are the same everywhere? Would be nice for backwards-compatibility.

It can be different if migrations are rolled back and reapplied because the sequence for the incrementing IDs is not reset during rollback.

mzur · 2026-09-02T13:55:59Z

This should be called something like replace_role_table_with_enum. If it's called remove_roles it implies something completely different.

mzur · 2026-09-02T13:57:10Z

+        // TODO Discuss if we want to "be safe" by mapping here or if it's enough
+        // to check in psql/with the migrations that the enum used the same numbers
+        // as the db table, in which case mapping is unnecessary


The mapping is essential for all static models (see above).

mzur · 2026-09-02T13:58:12Z

+                    ->update([$column => $newId]);
+            }
+
+            Schema::table($table, fn (Blueprint $t) => $t->dropForeign([$column]));


Isn't this required before you update the IDs to something potentially invalid above?

mzur · 2026-09-02T13:59:58Z

    biigle.$declare('labelTrees.members', {!! $members !!});
    biigle.$declare('labelTrees.roles', {!! $roles !!});
-    biigle.$declare('labelTrees.defaultRole', {!! Biigle\Role::editor() !!});
+    biigle.$declare('labelTrees.defaultRole', @json(Biigle\Role::editor()->toArray()));


Maybe also add implements Stringable to the enums?

mzur · 2026-09-02T14:12:08Z

Regarding E2E integration tests: I'd like to stick with testing the backend/API only because I feel full UI/E2E tests would quickly become a burden to maintain with our little resources. If people start paying for BIIGLE to be reliable in business-critical applications, we can reconsider this.

Besides, since I just noticed this: In the future, please try to align your commit messages to the convention we use.

yannik131 · 2026-09-03T16:42:53Z

@mzur I agree with your suggestions, and I don't think it will take more than a couple of hours to add these other changes. Shouldn't be a problem.
But do feel free to change priorities of issues on the roadmap if you think some issues are more important than others. I always pick issues with higher priority first, and no issue currently has "low" priority.

mzur · 2026-09-04T08:12:19Z

I think this is not an issue of priorities (besides that "low" issues are basically never worked on), it's more my habit of writing down ideas as issues without always giving much though about gain/effort. So we have issues with medium priority that we definitely need sitting next to issues with the same priority that we should be more careful with. I'm not sure how to deal with it other than asking you to think about implications first and talking to me if you think gain/effort is too low.

P.S.: You find issues with low priority in the full roadmap. The task list is already the version filtered by me.

Removing role table from codebase

85f8776

yannik131 changed the title ~~Removing role table from codebase~~ Remove static models Aug 25, 2026

yannik131 added 9 commits August 26, 2026 13:27

further replacing roles with enum

6969cf0

further replacing roles with enum

d8c9c82

Fixed remaining role tests

9f75cb6

formatting

daccf56

fixed linter errors

0f62736

made down() recreate FKs

fcaf783

Moved Role to Enums folder

22157ad

composer fix

439e854

media_type enum and migration

e5718e4

yannik131 added 18 commits August 27, 2026 18:14

todo comments

09beb1a

replace MediaType with enum

ee19c9b

replacing media type with enum

c67a0b2

fixed tests (media type enum)

bda4219

fixed frontend bugs

d4988d1

composer lint & fix

c110ea2

Reversed enum folder creation

6762396

Will do that once the PR has been reviewed

shapes enum & migrations

91e3c41

Deleted factories

bf7101b

Working on fixing shape tests

e73c747

fixed last failing shape test

c229549

visiblity enum was very easy

b84216d

composer lint

b306951

Replacing report type with enum

888823e

remaining report type replacements

77d1b10

fixed linter errors

0b05f8e

fixed failing tests

5e78462

renamed role migration

b2d43c1

yannik131 marked this pull request as ready for review August 31, 2026 17:04

yannik131 requested a review from mzur August 31, 2026 17:13

yannik131 mentioned this pull request Sep 1, 2026

Replace static models with enums biigle/maia#241

Open

mzur reviewed Sep 2, 2026

View reviewed changes

	'shape_id' => ['required', 'integer', Rule::in(Shape::pluckById()->keys()->all())],
	'shape_id' => ['required', 'integer', Rule::enum(Shape::class)],

	$shapeIds = Shape::pluckById(Shape::wholeFrame())->keys();
	$shapeIds = Shape::pluckById(except: Shape::wholeFrame())->keys();

Uh oh!

Conversation

yannik131 commented Aug 25, 2026

Uh oh!

yannik131 commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Migration guide for me

Enum implementation

Creating the migration

Editing old migrations

Common needed replacements

Uh oh!

yannik131 commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzur left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yannik131 Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzur commented Sep 2, 2026

Uh oh!

yannik131 commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzur commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

yannik131 commented Aug 27, 2026 •

edited

Loading

yannik131 commented Aug 31, 2026 •

edited

Loading

yannik131 Sep 2, 2026 •

edited

Loading

yannik131 commented Sep 3, 2026 •

edited

Loading

mzur commented Sep 4, 2026 •

edited

Loading