Skip to content
GitLab
  • Menu
Projects Groups Snippets
  • /
  • Help
    • Help
    • Support
    • Community forum
    • Submit feedback
    • Contribute to GitLab
  • Sign in / Register
  • D debexpo
  • Project information
    • Project information
    • Activity
    • Labels
    • Members
  • Repository
    • Repository
    • Files
    • Commits
    • Branches
    • Tags
    • Contributors
    • Graph
    • Compare
  • Issues 42
    • Issues 42
    • List
    • Boards
    • Service Desk
    • Milestones
  • Merge requests 2
    • Merge requests 2
  • CI/CD
    • CI/CD
    • Pipelines
    • Jobs
    • Schedules
  • Deployments
    • Deployments
    • Environments
    • Releases
  • Monitor
    • Monitor
    • Incidents
  • Analytics
    • Analytics
    • Value stream
    • CI/CD
    • Repository
  • Snippets
    • Snippets
  • Activity
  • Graph
  • Create a new issue
  • Jobs
  • Commits
  • Issue Boards
Collapse sidebar
  • mentors.debian.net
  • debexpo
  • Merge requests
  • !115

django: Fix model validation

  • Review changes

  • Download
  • Email patches
  • Plain diff
Merged Baptiste Beauplat requested to merge lyknode/debexpo:fix/model-validation into django Mar 26, 2020
  • Overview 3
  • Commits 4
  • Pipelines 3
  • Changes 10

Django migration: #47 (closed)

tldr; MR to add object validation before saving to the DB.

I attempted to update https://debexpo.d.n to the latest version of the django branch, integrating the plugin MR. While importing a new package to test the distribution plugin, I ran into a database integrity error:

2020-03-25 17:45:58 UTC [19221-1] lyknode@lyknode-testing ERROR:  value too long for type character varying(32)
2020-03-25 17:45:58 UTC [19221-2] lyknode@lyknode-testing STATEMENT:  INSERT INTO "plugins_pluginresults" ("upload_id", "plugin", "test", "outcome", "json", "severity") VALUES (692, 'distribution', 'same-distribution-changes-and-changelog', 'changes and changelog distribution differs', '{"changes": "stable", "changelog": "unstable"}', 3) RETURNING "plugins_pluginresults"."id"

Imaging my surprise! I had the plugin test covered for this case. No way I could not have run into that particular issue while manually testing and while writing the tests. But here we are, the blunt truth spoken (written?) by that log.

Let's backtrack a bit and try to understand what happened. First the DBMS. I use a SQlite backend locally to avoid running a full fledged service. Let's have a look at the tables:

The definition:

CREATE TABLE IF NOT EXISTS "plugins_pluginresults" ("id" integer NOT NULL PRIMARY KEY AUTOINCREMENT, "plugin" text NOT NULL, "test" varchar(32) NOT NULL, "outcome" text NOT NULL, "json" text NULL, "severity" smallint unsigned NOT NULL CHECK ("severity" >= 0), "upload_id" integer NOT NULL REFERENCES "packages_packageupload" ("id") DEFERRABLE INITIALLY DEFERRED);
CREATE INDEX "plugins_pluginresults_upload_id_256cc125" ON "plugins_pluginresults" ("upload_id");

Test is indeed a VARCHAR(32). Mmm. The data then?

> SELECT test FROM plugins_pluginresults LIMIT 1;
same-distribution-changes-and-changelog
> SELECT LENGTH(test) FROM plugins_pluginresults LIMIT 1;
39

Right. 👏 👏 👏 Thanks SQLite. The reason behind that madness is that SQlite actually have a small subset to SQL types. VARCHAR being translated to TEXT. I guess django uses SQLite as well for in-memory database for the test, explaining why it did not show up.

Let's wrap around that. The other issue I take with all of that is: What the hell am I writing models for? Isn't django supposed to warn me about such integrity errors before actually trying to push to the DB?

Well, I was under the wrong assumption that Object.save() would validate the model. I had seen that working fine with all Forms from the UI, being validated and then save to the DB. However I had been tricked.

This is what we have for a Form:

if form.is_valid():
    form.save()

Guess what? is_valid() does do exactly what is says... It does validate against the model. But, that being only applicable to the Forms, other objects created in the backend are supposed to be checked with Object.full_clean(), as clearly stated in the documentation.

All of that text to present this MR, that add the full_clean() when needed, and fixes the initial data and models.

That was fun, sorry for the long description 🥔.

Assignee
Assign to
Reviewer
Request review from
Time tracking
Source branch: fix/model-validation