Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch multi throws NotFound error if new data doesn't match original query #92

Closed
robbyphillips opened this issue Apr 23, 2020 · 4 comments

Comments

@robbyphillips
Copy link
Contributor

I wrote a test case that uses this library's testing infrastructure. Should be able to just add it to the testing directory.

import { expect } from 'chai';
import feathers from '@feathersjs/feathers';
import knex from 'knex';
import service from '../src';
import People from './people';
import { Model } from 'objection';

const db = knex({
  client: 'sqlite3',
  debug: false,
  connection: {
    filename: './db.sqlite'
  },
  useNullAsDefault: false
});

// Bind Objection.js
Model.knex(db);

const app = feathers().use(
  '/people',
  service({
    model: People,
    id: 'id',
    multi: ['create', 'patch'],
    whitelist: ['$and', '$like', '$between', '$notBetween'],
    events: ['testing']
  })
);

const people = app.service('people');

function clean (done) {
  db.schema.dropTableIfExists('people').then(() => {
    return db.schema
      .createTable('people', (table) => {
        table.increments('id');
        table.string('name');
        table.integer('age');
        table.integer('time');
        table.boolean('created');
      })
      .then(() => done());
  });
}

describe('Additional Patch Test', () => {
  before(clean);
  after(clean);

  beforeEach(async () => {
    await people.create([
      {
        id: 1,
        name: 'Ahmed',
        age: 35
      },
      {
        id: 2,
        name: 'Hans'
      },
      {
        id: 3,
        name: 'Samantha'
      }
    ]);
  });

  afterEach(async () => {
    try {
      await people.remove(1);
    } catch (err) {}
    try {
      await people.remove(2);
    } catch (err) {}
    try {
      await people.remove(3);
    } catch (err) {}
  });

  it('patch with null id and query', () => {
    return people
      .patch(
        null,
        { age: 42 },
        {
          query: {
            age: null
          }
        }
      )
      .then((data) => {
        expect(data.length).to.be.equal(2);
      });
  });
});

Expected

We should be able to patch multiple items and get back the changed items, even if the data changed is in the original query.

What happens?

If the data used to find the items in the original query changes, the patch operation succeeds in changing the items in the database, but then fails and throws a NotFound: No record found of id 'null error, rather than returning the data.

This is breaking functionality like "soft delete" for me.

@dekelev
Copy link
Member

dekelev commented Apr 24, 2020

Hi @robbyphillips , I've added the $noSelect query operator to be used also for use-cases like yours. Check it out.

@dekelev dekelev closed this as completed Apr 24, 2020
@robbyphillips
Copy link
Contributor Author

Is this the expected behavior, though? All other patch operations return the modified data.

@dekelev
Copy link
Member

dekelev commented Apr 26, 2020

It is an expected behavior in the sense that it has always behaved this way, but the behavior could not align with the expected behavior from FeathersJS database adapters.

I'll keep the ticket opened until I can verify this.

@dekelev dekelev reopened this Apr 26, 2020
@dekelev
Copy link
Member

dekelev commented May 9, 2020

Thanks @robbyphillips, v5.4.0 was released.

Patch with same field in data & query now returns success result.

@dekelev dekelev closed this as completed May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants