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

fix(archives): metadata labels may be set during upload #1063

Merged
merged 3 commits into from
Sep 15, 2022

Conversation

lkonno
Copy link
Contributor

@lkonno lkonno commented Sep 12, 2022

Fixes #1045

The labels are set to the archived recording when "metadata" in JSON (e.g.: {"reason":"test","reason1":"test1"}) containing the labels is sent along to the uploaded file.

@lkonno lkonno changed the title Setting the metadata along to the file upload fix(archives) Setting the metadata along to the file upload Sep 12, 2022
@lkonno lkonno changed the title fix(archives) Setting the metadata along to the file upload fix(archives): Setting the metadata along to the file upload Sep 12, 2022
@andrewazores andrewazores changed the title fix(archives): Setting the metadata along to the file upload fix(archives): metadata labels may be set during upload Sep 13, 2022
@andrewazores
Copy link
Member

andrewazores commented Sep 13, 2022

There is a failing unit test that needs correcting or updating. Also, I think you will need to run mvn spotless:apply to apply automatic source code formatting.

How do I test this manually? I tried with ex.

$ curl -k -v -X POST --header "Authorization: Basic $(echo user:pass | base64)" -F recording=@$HOME/Downloads/es-andrewazor-demo-Main_foo_20220913T134900Z.jfr https://localhost:8181/api/v1/recordings -F metadata='{"labels":{"foo":"bar"}}'

and the request seems to hang. If I Ctrl-C the request then I can see that the recording file actually does exist in the archives but it does not have the metadata labels set.

@lkonno
Copy link
Contributor Author

lkonno commented Sep 13, 2022

To test it use:

$ curl -k -v -X POST --header "Authorization: Basic $(echo user:pass | base64)" -F recording=@$HOME/Downloads/es-andrewazor-demo-Main_foo_20220913T134900Z.jfr https://localhost:8181/api/v1/recordings -F metadata='{"foo":"bar"}'

Or multiple labels:

$ curl -k -v -X POST --header "Authorization: Basic $(echo user:pass | base64)" -F recording=@$HOME/Downloads/es-andrewazor-demo-Main_foo_20220913T134900Z.jfr https://localhost:8181/api/v1/recordings -F metadata='{"foo":"bar","foo1":"foo1"}'

I tested it from web-client editing the Api.services.tsx with:

  uploadRecording(file: File, labels: RecordingLabel[], signal?: AbortSignal): Observable<string> {
    window.onbeforeunload = () => true;
    return this.login.getHeaders().pipe(
      concatMap(headers => {
        const body = new window.FormData();
        body.append('recording', file);
        body.append('metadata', this.stringifyRecordingLabels(labels));

@andrewazores
Copy link
Member

Ah okay nice, that curl command worked! I had assumed that the nested labels object was required, but I was wrong. However, when I do that, I think that should either be ignored or result in a 400 response - it seems to just silently hang. I inserted some logger lines to see what's happening and it looks like the parseRecordingLabels call may be the culprit.

@andrewazores
Copy link
Member

Also using httpie, https -f -vv :8181/api/v1/recordings Authorization:"Basic $(echo user:pass | base64)" metadata='{"foo":"bar"}' recording@$HOME/Downloads/es-andrewazor-demo-Main_foo_20220913T134900Z.jfr works too. Nice.

@lkonno
Copy link
Contributor Author

lkonno commented Sep 14, 2022

Regarding the field name to input the labels, I think that using metadata can be confusing, so I am changing the name to labels.

$ curl -k -v -X POST --header "Authorization: Basic $(echo user:pass | base64)" -F recording=@$HOME/Downloads/es-andrewazor-demo-Main_foo_20220913T134900Z.jfr https://localhost:8181/api/v1/recordings -F labels='{"foo":"bar"}'

@lkonno
Copy link
Contributor Author

lkonno commented Sep 14, 2022

  • Invalid labels format now returns Bad Request and do not upload the recording file;
  • Fixed the test to check the metadata in the response;
  • The request now uses labels field name instead of metadata which is a not required field.

@andrewazores
Copy link
Member

Great work, thanks. The implementation looks good and works nicely in manual testing now. The updated test case looks good, too. Could you add a few more tests? Right now the unit test is only exercising the one case where the labels attribute is provided and has valid contents. I'd like to see a test that includes no labels and a few different cases where labels are invalid in various ways.

@lkonno
Copy link
Contributor Author

lkonno commented Sep 15, 2022

shouldHandleRecordingUploadRequest --> upload containing no labels
shouldHandleRecordingUploadRequestWithLabels --> Created to handle upload containing valid labels
shouldHandleInvalidLabels --> Created to handle invalid labels

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

Successfully merging this pull request may close these issues.

[Bug] Archived recording uploads do not handle recording metadata (labels)
2 participants