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

Support for unique index violation proper response instead of 500 #3258

Closed
amrsalama opened this issue Jun 27, 2019 · 9 comments
Closed

Support for unique index violation proper response instead of 500 #3258

amrsalama opened this issue Jun 27, 2019 · 9 comments

Comments

@amrsalama
Copy link
Contributor

Suggestion

Unique index violation (for example) in mongodb issues mongo driver error 'E11000 duplicate key error collection: loopback4.users index: username_1 dup key: { : "user" }'
and rejected with status code 500 response.

It would be nice if we have proper response (for example) 422 instead of 500.

I know that I can customize Sequence and handle it in catch section before reject. But can we have it auto rejected with proper status code instead? Like any other validation rejection.

Use Cases

  • Unique validation error.

Acceptance criteria

TBD - will be filled by the team.

@dougal83
Copy link
Contributor

This is dependant on the data connector in use, what about others? That aside you can customise the model/controller to emit the error feedback that you require(like 409 Conflict). See example below:

loopback4-example-shopping model declaration
loopback4-example-shopping controller

I had the same concern as you but submitted the above solution as a compromise. The constraint was placed on the model to give a specific name to check against. If you have a better alternative I'd love to hear your ideas.

@amrsalama
Copy link
Contributor Author

@dougal83 Thanks for your reply. You are right about the the connector dependency and I think this is the big issue to handle.

I think handling the problem in controller may leads to duplicate codes (in POST, PUT, PATCH).
My approach for now is to handle it in the Sequence

async handle(context: RequestContext) {
  try {
    const {request, response} = context;
    const route = this.findRoute(request);
    const args = await this.parseParams(request, route);
    const result = await this.invoke(route, args);
    this.send(response, result);
  } catch (err) {
    if (err.name === 'MongoError' && err.code === 11000) {
      Object.assign(err, {
        statusCode: 422,
        name: 'UnprocessableEntityError',
        message:
          'The request body is invalid. See error object `details` property for more info.',
        code: 'VALIDATION_FAILED',
      });
    }
    this.reject(context, err);
  }
}

But I need to reject with identical loopback response without writing message, code, and details myself, any ideas ?

@dougal83
Copy link
Contributor

dougal83 commented Jul 1, 2019

@amrsalama Ultimately you are limited by the data connector, which can always be made more flexible.

Is anything stopping you making a universal block of error handling code and reusing it (DRY)?:

    if (err.name === 'MongoError' && err.code === 11000) {
        throw new HttpErrors.UnprocessableEntity('message')
    }

Each to their own but I'll mostly be verbosely describing my controllers as it fits my usage.

@amrsalama
Copy link
Contributor Author

@dougal83

    try {
      // create the new user
      const savedUser = await this.userRepository.create(user);
      delete savedUser.password;

      return savedUser;
    } catch (error) {
      // MongoError 11000 duplicate key
      if (error.code === 11000 && error.errmsg.includes('index: uniqueEmail')) {
        throw new HttpErrors.Conflict('Email value is already taken');
      } else {
        throw error;
      }
    }

Here we use try/catch inside controller to handle the uniqueness rejection by Mongo (Inner). And we have another try/catch block in Sequence (Outer).

But it would be nicer if we can handle this problem in the outer try/catch in Sequence.

I can't throw exception in catch block of the Sequence (app would crash and stop).

@Archer-Thane
Copy link

I have run the loopback4-example-shopping and run serveral times post /users and there are no error with code 11000.
take a look at this picture.

@dougal83
Copy link
Contributor

dougal83 commented Dec 10, 2019

I have run the loopback4-example-shopping and run serveral times post /users and there are no error with code 11000.
take a look at this picture.

Hi Archer

Can you please expand the User collection in the db and confirm that the UniqueEmail constraint exists?

image

EDIT: My first thought is that you might not have run the db migration. If that is not familiar please read the Database migration documents and see if it helps.

@dougal83
Copy link
Contributor

But it would be nicer if we can handle this problem in the outer try/catch in Sequence.

I can't throw exception in catch block of the Sequence (app would crash and stop).

@amrsalama Apologies for missing this message. I see your point, there is no one right way to handle this, but I simply implemented the most expedient solution. Feel free to submit a pull request to replace this code if you have an alternative.

@deepakrkris deepakrkris self-assigned this May 7, 2020
@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Jul 14, 2021
@stale
Copy link

stale bot commented Aug 13, 2021

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants