Skip to content

2221/config update 500#2222

Merged
NealSun96 merged 16 commits intoapache:masterfrom
micahstubbs:2221/config-update-500
Sep 26, 2022
Merged

2221/config update 500#2222
NealSun96 merged 16 commits intoapache:masterfrom
micahstubbs:2221/config-update-500

Conversation

@micahstubbs
Copy link
Contributor

@micahstubbs micahstubbs commented Sep 20, 2022

Description

This PR fixes a bug that occurs because the request body is now ignored silently by the Angular http library if it is passed as stringified JSON. Now, all request bodies are passed as objects, which the Angular http library then stringifies itself.

This PR also passes the http response status code from the helix-front web server to the UI, so that response codes from the helix-rest like 400 Bad Request are shown in the UI. This helps the user better understand what is going wrong.

This PR also ensures that the response sent from helix-front to helix-rest is identical to the working curl request.

Here is the desired payload format, modeled after the working curl request:

{
  "id": "my-test-cluster",
  "mapFields": { "TEST_FIELD": { "test_sub_field": 18 } }
}

Here is the bug format seen before, which differed slightly from the curl request:

{
  "id": "my-test-cluster",
  "mapFields": { "TEST_FIELD": { "test_sub_field": "18" } },
  "listFields": {},
  "simpleFields": {}
}

Notice the following differences:

  1. properties with empty object values are included
  2. the number value of "test_sub_field" is a string, not a number.

Fix #2221

Tests

TODO: add a test for this case.

Code Style

Formatted using Prettier

console.log(realUrl);
console.log('');
console.log('request body');
console.log(req.body);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to leave this logging in the server logs, to aid debugging similar issues in the future. I'm happy to modify the formatting if reviewers recommend this.

Choose a reason for hiding this comment

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

Not against this but one option here is to gate this behind a local storage toggle for debugging. It would prevent unnecessarily logging at all times.

@micahstubbs micahstubbs force-pushed the 2221/config-update-500 branch from 14befce to 65289f6 Compare September 21, 2022 18:27
@micahstubbs micahstubbs changed the title WIP 2221/config update 500 2221/config update 500 Sep 21, 2022
Copy link

@somecodemonkey somecodemonkey left a comment

Choose a reason for hiding this comment

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

Minor comments

console.log(realUrl);
console.log('');
console.log('request body');
console.log(req.body);

Choose a reason for hiding this comment

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

Not against this but one option here is to gate this behind a local storage toggle for debugging. It would prevent unnecessarily logging at all times.

request[method](options, (error, response, body) => {
if (error) {
res.status(500).send(error);
res.status(response.statusCode || 500).send(error);

Choose a reason for hiding this comment

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

Is there ever a case where the statusCode will be empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, will handle this.

@micahstubbs
Copy link
Contributor Author

This PR is ready to be merged, approved by @somecodemonkey
Final commit message:

Ensure request body is sent from helix-front to helix-rest (#2221 )

Fix Angular http payload argument bug
Display helix-rest status codes in Helix UI

@NealSun96 NealSun96 merged commit 83e9bab into apache:master Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

500 Internal Server Error response from helix-rest when updating cluster config

3 participants