Vincit / objection.js

An SQL-friendly ORM for Node.js
https://vincit.github.io/objection.js
MIT License
7.24k stars 636 forks source link

$query().patch() with conditionals in jsonSchema fails to validate properly #884

Closed falkenhawk closed 6 years ago

falkenhawk commented 6 years ago

I am getting a validation error when using if-then-else conditionals in jsonSchema. But not on insert, but only on patch() so far. I am not sure if it's a bug in objection or maybe in ajv? Tested on v1.0.0 and v1.1.6. Only downgrading to v0.9.4 fixes the error, but I am not sure then if if-then-else conditionals are checked in that version at all.

Reproduction case below. It seems that firstName null somehow meets the if firstName = 'Test' condition...

/**
 * This is a simple template for bug reproductions. It contains three models `Person`, `Animal` and `Movie`.
 * They create a simple IMDB-style database. Try to add minimal modifications to this file to reproduce
 * your bug.
 *
 * install:
 *    npm install objection knex sqlite3 chai
 *
 *
 * run:
 *    node reproduction-template
 */

let Model;

try {
  Model = require('./').Model;
} catch (err) {
  Model = require('objection').Model;
}

const Knex = require('knex');
const chai = require('chai');

async function main() {
  await createSchema();

  ///////////////////////////////////////////////////////////////
  // Your reproduction
  ///////////////////////////////////////////////////////////////

  const person = await Person.query().insertAndFetch({
    firstName: null,
    lastName: 'Lawrence',
  });

  await person.$query().patch({
    lastName: 'bla',
  });

  chai.expect(1).to.equal(1);
}

///////////////////////////////////////////////////////////////
// Database
///////////////////////////////////////////////////////////////

const knex = Knex({
  client: 'sqlite3',
  useNullAsDefault: true,
  connection: {
    filename: ':memory:'
  }
});

Model.knex(knex);

///////////////////////////////////////////////////////////////
// Models
///////////////////////////////////////////////////////////////

class Person extends Model {
  static get tableName() {
    return 'Person';
  }

  static get jsonSchema() {
    return {
      type: 'object',
      required: ['lastName'],

      properties: {
        id: {type: 'integer'},
        firstName: {type: ['null', 'string'], minLength: 1, maxLength: 255},
        lastName: {type: 'string', minLength: 1, maxLength: 255},
        age: {type: 'number'},

        address: {
          type: 'object',
          properties: {
            street: {type: 'string'},
            city: {type: 'string'},
            zipCode: {type: 'string'}
          }
        }
      },

      if: {
        properties: {
          firstName: { enum: ['Test'] },
        },
      },
      then: {
        required: ['age'],
      },
      else: {
        properties: {
          age: { type: 'null' },
        },
      },
    };
  }
}

///////////////////////////////////////////////////////////////
// Schema
///////////////////////////////////////////////////////////////

async function createSchema() {
  await knex.schema
    .dropTableIfExists('Person');

  await knex.schema
    .createTable('Person', table => {
      table.increments('id').primary();
      table.string('firstName');
      table.string('lastName');
      table.integer('age');
      table.json('address');
    });
}

main()
  .then(() => console.log('success'))
  .catch(console.error);
Error: age: is a required property, : should match "then" schema
koskimas commented 6 years ago

patch attempts to ignore required properties, but it doesn't handle if yet. I'll try to fix this.

koskimas commented 6 years ago

@unhawkable I just released 1.1.7. This should be fixed in it.

falkenhawk commented 6 years ago

Thank you. Your level of support is far beyond any expectations. 🥇 As a side note, do you think also "else" keyword should be added to subSchemaProps? And there is also a special property "dependencies" which can contain "required" prop in it, but can also take a short form of "dependencies": { "foo": ["bar"] } more about it: https://stackoverflow.com/a/38781027

koskimas commented 6 years ago

@unhawkable Thanks for the kind words!

I already added "else" to the list. Thanks for the tip about "dependencies". I'll see what I can do about that one.