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

Return HTTP 400 with an informative message when metadata to be published #335

Merged
merged 1 commit into from Aug 22, 2019

Conversation

mjgiarlo
Copy link
Member

@mjgiarlo mjgiarlo commented Aug 22, 2019

Fixes #331

@ndushay This is a very specific fix to #331. I'm not sure if you were hoping for a more general (across-the-board) fix. I'm probably inclined to fix the narrowest case and add more error handling as we find other exceptions that don't bubble up useful information, but I am open to pushback on this if you think it's too short-sighted.

@mjgiarlo mjgiarlo added this to Needs Review in Infrateam Aug-Sep 2019 via automation Aug 22, 2019
@coveralls
Copy link

Coverage Status

Coverage increased (+0.006%) to 96.477% when pulling 1da1fd2 on surface_errors_331 into 913ab5f on master.

@justinlittman justinlittman merged commit f7183ae into master Aug 22, 2019
Infrateam Aug-Sep 2019 automation moved this from Needs Review to Done Aug 22, 2019
@mjgiarlo mjgiarlo deleted the surface_errors_331 branch August 22, 2019 18:27
@ndushay
Copy link
Member

ndushay commented Aug 22, 2019

I was thinking more broadly, as, as stated in the ticket (which this PR closed, ahem), I believe we can possibly change a rails setting and get more info from the specific error.

Should I re-open the ticket?

@ndushay
Copy link
Member

ndushay commented Aug 22, 2019

And, by the way, it was a 500 error for the publish step, so not sure this even fixes that.

@mjgiarlo
Copy link
Member Author

@ndushay It does fix the 500 error, which was caused by a lack of more specific exception handling in the app.

@@ -11,6 +11,10 @@ class ObjectsController < ApplicationController
render status: 409, plain: e.message, location: object_location(e.pid)
end

rescue_from(DublinCoreService::CrosswalkError) do |e|
render status: 400, plain: e.message
Copy link
Member

Choose a reason for hiding this comment

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

400 is misleading -- there is nothing wrong with the REQUEST -- it is the dor object empty descMetadata that is the problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

dor-services-app errors raised need to surface more info to caller
4 participants