search

NCEAS / metacat

Data repository software that helps researchers preserve, share, and discover data
https://knb.ecoinformatics.org/software/metacat
GNU General Public License v2.0
26 stars 12 forks source link

Unhandled HTTP 500 "String index of out of range" error #1495

Closed amoeba closed 2 years ago

amoeba commented 3 years ago

I typo'd a Metacat URL and ended up with an unhandled except that really oughta be handled with a 400.

To reproduce:

  1. Go to https://knb.ecoinformatics.org/knb/metacat/d1/mn/v2/meta/foo
  2. See HTTP Status 500 – Internal Server Error: java.lang.StringIndexOutOfBoundsException: String index out of range: -1

You can see that the URL I meant to type was https://knb.ecoinformatics.org/knb/d1/mn/v2/meta/foo (w/o 'metacat' after 'knb').

mbjones commented 3 years ago

Agreed. Good catch. Any non-existent service paths should return 404 rather than 500. @taojing2002 can you patch this up when convenient in an upcoming release? Sounds like it needs some better exception handling in the dispatch methods. Probably another case where we're catching Exception rather than dealing with different exception types properly.

taojing2002 commented 2 years ago

@amoeba The URL of https://knb.ecoinformatics.org/knb/metacat/d1/mn/v2/meta/foo has the the keyword metacat. So it goes to the old Metacat API rather than the DataONE API. So it is an old Metacat API issue.

amoeba commented 2 years ago

Thanks for taking a look, @taojing2002. That explanation makes sense.

However, I think that it's still a bug here. I realize this is subjective, but I think it's an ultimate goal that public-facing HTTP endpoints to never 500 as 500s are for exceptional and unhandled behavior. Parsing and handling an HTTP request shouldn't really result in exceptional behavior. I think we can produce a 400-series code here, whatever's appropriate. Probably 404.

I'd be happy to take a look if you'd like. Let me know.

taojing2002 commented 2 years ago

Yeah. I agree it is still a bug and I can take a look. On 10/18/21 2:53 PM, Bryce Mecum wrote:

Thanks for taking a look, @taojing2002 https://github.com/taojing2002. That explanation makes sense.

However, I think that it's still a bug here. I realize this is subjective, but I think it's an ultimate goal that public-facing HTTP endpoints to never 500 as 500s are for exceptional and unhandled behavior. Parsing and handling an HTTP request shouldn't really result in exceptional behavior. I think we can produce a 400-series code here, whatever's appropriate. Probably 404.

I'd be happy to take a look if you'd like. Let me know.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NCEAS/metacat/issues/1495#issuecomment-946196447, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB5QQDF25IVC56EJYTJF56TUHSJMRANCNFSM4ZV4HGIQ. Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

amoeba commented 2 years ago

Thanks @taojing2002