-
Notifications
You must be signed in to change notification settings - Fork 22
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: Handles operation arguments imported as strings #546
Conversation
Test Results 8 files + 1 8 suites +1 59m 39s ⏱️ + 9m 27s For more details on these failures, see this check. Results for commit 69be64f. ± Comparison against base commit 03ea6c9. This pull request removes 4 and adds 2 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Test resultsRun attempt: 361
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide us with an example of Jolokia requests that you want to address with the fix? I wonder how it happens in the first place.
const args: OperationArgument[] = [] | ||
for (const arg of op.args) { | ||
let argObj = arg | ||
if (!isObject(arg)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still not clear to me why this check is needed. The arg type is defined like this:
https://github.com/hawtio/jolokia.js/blob/dc6bb0c8c8c1f1904b58630c8a1aab9e69ac2f36/index.d.ts#L65-L69
Maybe you can provide us with an example of Jolokia requests that are irregular to the defined type, then we can modify the type definition accordingly. Or it could be a bug in Jolokia that should rather be raised to Jolokia project.
try { | ||
argObj = JSON.parse(arg as string) | ||
} catch (error) { | ||
log.error(`Cannot parse argument ${arg} in operation ${op.desc}`, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oftentimes op.desc
doesn't provide enough information, so it's better to use name
here.
Also, the code hasn't been changed so much from v2. I wonder why it wasn't a case with the old Hawtio v2. (Or it was?) private addOperation(operations: Operation[], operationMap: { [name: string]: Operation },
opName: string, op: { args: any[], desc: string }): void {
let operation = new Operation(
opName,
op.args.map((arg) => _.assign(new OperationArgument(), arg)),
op.desc);
operations.push(operation);
operationMap[operation.name] = operation;
} |
I added some console log messages to the existing source to diagnose the problem - see screenshot.
The reason for the exception is that The console log confirms displays (see 1) that the arg array normally contains properties objects. However, in the case of 'Dumps the route as XML' operation, the arg value is defined as a string (see 2). It seems to be the only arg that is in this form so it may be an error in the dependencies. If it is a bug further down the stack, then it is probably worth investigating and fixing. However, I think it cautious to check the arguments rather than assume. That was my thinking here. |
operation-bug.mp4Thought a quick video might clarify the process. |
@phantomjinx Can you reproduce the issue with the old Hawtio as well? |
@phantomjinx I cannot reproduce the issue at my end, with either the current
Actually, you don't need to insert In my case, both And talking about the fix, if this issue indeed happens in some case and needs to be fixed, I think we should rather fix it earlier than when an operation object is instantiated, somewhere like here: hawtio-next/packages/hawtio/src/plugins/shared/workspace.ts Lines 70 to 85 in 03ea6c9
|
Curiously no. The Operation args seem to come through correctly. |
The test application, forked from the main quick-starts, is located here -> https://github.com/phantomjinx/spring-boot-camel/tree/fuse-7.x.redhat |
* Some Operation Arguments retrieved by jolokia are surrounded with quotes hence are handled internally as strings. * Using the current mechanism only populates the arg.name parameter, leaving arg.type null, resulting in some ugly errors when OperationArgument is constructed. * By checking whether the arg is an Object and if not parsing it mitigates the problem of the quotes
d1b6cfe
to
69be64f
Compare
@phantomjinx I don't reproduce the issue even with the quickstart you use. I guess you're deploying it to OpenShift and trying to connect to it from hawtio-online, aren't you? Then it should be the issue with the way hawtio-online processes the returned mbeans at its in-the-middle nginx server. See: https://github.com/hawtio/hawtio-online/blob/2.x/docker/nginx.js If that's the case, let's close this one and instead file this issue to hawtio-online and fix it there. |
Closing as discussed. Has not proven to be the case elsewhere and if it occurs again on |
Some Operation Arguments retrieved by jolokia are surrounded with quotes hence are handled internally as strings.
Using the current mechanism only populates the arg.name parameter, leaving arg.type null, resulting in some ugly errors when OperationArgument is constructed.
By checking whether the arg is an Object and if not parsing it mitigates the problem of the quotes