Skip to content

Conversation

@jozefcipa
Copy link
Contributor

@jozefcipa jozefcipa commented Oct 16, 2020

What does it do?

This PR handles duplicate errors occurred on unique columns in the database and shows a user-friendly message, instead of a raw database error.

Before
image

After
image

Fixes #6915, #8664

@jozefcipa jozefcipa requested a review from a team October 16, 2020 22:13
@jozefcipa jozefcipa requested a review from a team as a code owner October 16, 2020 22:14
@codecov
Copy link

codecov bot commented Oct 16, 2020

Codecov Report

Merging #8367 (3d605a7) into master (5bbb8e6) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8367      +/-   ##
==========================================
- Coverage   34.64%   34.64%   -0.01%     
==========================================
  Files        1308     1308              
  Lines       14431    14437       +6     
  Branches     1432     1432              
==========================================
+ Hits         5000     5001       +1     
- Misses       8517     8522       +5     
  Partials      914      914              
Flag Coverage Δ
front 26.04% <ø> (-0.01%) ⬇️
unit 54.87% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...kages/strapi-admin/admin/src/translations/index.js 100.00% <0.00%> (ø)
...trapi-plugin-email/admin/src/translations/index.js 0.00% <0.00%> (ø)
...rapi-plugin-upload/admin/src/translations/index.js 0.00% <0.00%> (ø)
...in-content-manager/admin/src/translations/index.js 0.00% <0.00%> (ø)
...-users-permissions/admin/src/translations/index.js 0.00% <0.00%> (ø)
...ntent-manager/admin/src/components/Search/index.js 0.00% <0.00%> (ø)
...ntent-type-builder/admin/src/translations/index.js 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35835fd...3d605a7. Read the comment docs.

Signed-off-by: jozefcipa <jozef.cipa@strv.com>
Signed-off-by: jozefcipa <jozef.cipa@strv.com>
Copy link
Member

@derrickmehaffy derrickmehaffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why the @strapi/documentation got tagged for review, but if it's related to the translations, they look fine to me.

@strapi-cla
Copy link

strapi-cla commented Nov 30, 2020

CLA assistant check
All committers have signed the CLA.

@alexandrebodin
Copy link
Member

Hi ! @jozefcipa this looks really promising :) can you fix the conflicts on your PR ?

@alexandrebodin alexandrebodin added source: core:database Source is core/database package issue: enhancement Issue suggesting an enhancement to an existing feature labels Dec 14, 2020
@jozefcipa
Copy link
Contributor Author

@alexandrebodin fixed!

@derrickmehaffy derrickmehaffy self-requested a review December 17, 2020 21:58
@gddid
Copy link

gddid commented Jan 14, 2021

any update for this ? @jozefcipa

@jozefcipa
Copy link
Contributor Author

@gddid it is ready and waiting for a review. cc: @alexandrebodin

Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you compose the function instead of adding a try catch ?

// keep old implementation
const create = () => {}


return {
  create: wrapErrors(create),
  update: wrapErrors(update)
}


// utils
// keep your impl
const handleError = () => {}

// add a composable function
const wrapErrors = fn => async (...args) => {
  try {
    return fn(...args)
  } catch (error) {
    return handleErrors(error)
  }
}

@jozefcipa
Copy link
Contributor Author

I think I finally fixed the tests but when I went to test the fix in the administration I noticed it might be already being handled (strapi-plugin-content-manager/utils/wrap-bad-request.js:13) 🤔
So now I'm not sure if it makes sense to merge this PR.

image

@alexandrebodin
Copy link
Member

alexandrebodin commented Jan 22, 2021

I think I finally fixed the tests but when I went to test the fix in the administration I noticed it might be already being handled (strapi-plugin-content-manager/utils/wrap-bad-request.js:13) 🤔
So now I'm not sure if it makes sense to merge this PR.

image

What you did will give the right error when using the queries programmaticaly which the wrap-bad-request doesn't do so still worth it :)

@alexandrebodin alexandrebodin added this to the 3.4.5 milestone Jan 22, 2021
Copy link
Member

@alexandrebodin alexandrebodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, Thank you fro this contribution 💯

@alexandrebodin alexandrebodin merged commit 808b2a4 into strapi:master Jan 25, 2021
@jozefcipa jozefcipa deleted the fix/duplicate-entry-error branch January 25, 2021 13:04
@derrickmehaffy
Copy link
Member

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/new-release-strapi-v3-4-5/2446/1

@derrickmehaffy
Copy link
Member

This pull request has been mentioned on Strapi Community Forum. There might be relevant details there:

https://forum.strapi.io/t/custom-error-handling/199/5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

issue: enhancement Issue suggesting an enhancement to an existing feature source: core:database Source is core/database package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Status 500 Error message not clear for constraint validation

5 participants