openverse icon indicating copy to clipboard operation
openverse copied to clipboard

Audio waveform should return 424 instead of 500 when waveform cannot be generated

Open obulat opened this issue 9 months ago • 1 comments

Problem

When the waveform generation returns a non-zero exit status, we should return a 424 error on the /waveform endpoint, not 500.

https://api.openverse.engineering/v1/audio/e0742ff3-f909-4e1d-9555-8e6399157452/waveform/

returns

{
    "detail": "Command '['audiowaveform', '--input-filename', 'audio-e0742ff3-f909-4e1d-9555-8e6399157452.mp3', '--output-format', 'json', '--pixels-per-second', '22223']' returned non-zero exit status 1."
}

Description

Instead of returning the error message, we should log the error message and return a 424 exception similar to UpstreamThumbnailException: https://github.com/WordPress/openverse/blob/b55d6d521ee71b717f73ccf225b1de3bf35e5df9/api/api/utils/image_proxy/init.py#L200

Additional context

obulat avatar Apr 29 '24 06:04 obulat

Hi ! I'd like to check this one out if that's ok.

I have two questions though:

  1. I set up the dev environment, but there is no Audio record in the database that allows me to reproduce this behavior. Right now I'm just manually raising a ValueError so it enters the except block.
  2. You want a new custom exception class for this? I can just use the existing UpstreamThumbnailException and call it a day. Just thought Audio exceptions should have their own class to avoid confusion.

Hanimir-san avatar May 04 '24 17:05 Hanimir-san

Hi @Hanimir-san ! I've assigned you the issue, please let me know if you're no longer interested.

  1. Manually raising a ValueError is fine. Another option could be to manually change a direct URL of one of the audio records in sample_audio.csv and running just init again to load the new URL for manual testing. You can also see how the UpstreamThumbnailExtension is tested in automated tests here: https://github.com/WordPress/openverse/blob/e6fa7b359bed16af6e2a637f32fec30aed4816e7/api/test/unit/utils/test_image_proxy.py#L485-L493

  2. A new custom UpstreamWaveformException (with code 424) would be best.

obulat avatar May 06 '24 14:05 obulat

@Hanimir-san, thank you again for your interest here. I have unassigned this issue. If you would ever like to resume work on it, do not hesitate to let us know here.

obulat avatar Jun 04 '24 03:06 obulat