-
Notifications
You must be signed in to change notification settings - Fork 76
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
Serialize Service Mgmt API's report endpoint body correctly #3294
Serialize Service Mgmt API's report endpoint body correctly #3294
Conversation
db28217
to
1f102b8
Compare
const buildFormData = (formData: Record<string, boolean | number | string>, data: BodyValue, parentKey?: string) => { | ||
if (data && typeof data === 'object') { | ||
Object.keys(data).forEach((key: string) => { | ||
buildFormData(formData, data[key], parentKey ? `${parentKey}[${key}]` : key) | ||
}) | ||
} else { | ||
if (parentKey) { | ||
formData[parentKey] = data ? data : '' | ||
} | ||
} | ||
} |
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.
Several issues with this method:
I always find it very confusing two mix up declaration order, object comes as an argument and it's should be the focus of this function but instead is used at the very end for the first time. buildFormData
is complex enough to be declared elsewhere, in fact it can be extracted without refactoring since it does not depend on objectToFormData
.
The fact that an empty object is being populated from scratch suggests to me a reduce
could be used here, ideally:
formData = body.reduce(buildFormData, {})
Recurrence is nice provided the method signature doesn't change but here both data
and parentKey
generate control couples which IMHO means it should be split (at least) in two. On top of that formData
is empty during the first iteration colliding with the actual method signature:
Record<string, never>
// vs
Record<string, boolean | number | string>
There is not one test for this method. I've spent a fair amount of time looking at it and still cannot get the grasp of it so it will be hard to maintain. Considering its purpose is pretty straightforward I strongly suggest it be refactored. I'm working on an alternative right now, I really rather sacrifice verbosity for readability.
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.
I think buildFormData
is OK to be inside objectToFormData
. Otherwise the only thing objectToFormData
would do is to call buildFormData
, no need to create a new function for that. In any case, what I would do is remove objectToFormData
and call buildFormData({}, body)
directly from transformReportRequestBody
.
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.
The fact that an empty object is being populated from scratch suggests to me a reduce could be used here
That's what I thought initially, but the problem is that we do need to traverse the whole object tree, so I am not sure that reduce
would be that helpful (or easy to implement).
I'll address the rest of the comments later.
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.
I always find it very confusing two mix up declaration order, object comes as an argument and it's should be the focus of this function but instead is used at the very end for the first time.
Well, the only reason why it is used in the end is because buildFormData
is declared inside. I can extract it from there, no problem. It's just that I think thought of objectToFormData
as a function that does everything, I think it's pretty useless if we just keep the last 3 lines (basically, I agree with what @jlledom says, except I don't think we need to call buildFormData({}, body)
directly from transformReportRequestBody
.
Anyway, I will see if I can make it more readable.
buildFormData
is complex enough to be declared elsewhere, in fact it can be extracted without refactoring since it does not depend on objectToFormData.
But do we need to have an exported function somewhere else (where?) if we know it is not going to be used by anything rather than this function? To me it makes more sense to keep the things together in this case, this will avoid the need to jump around multiple files/lines in the same file to understand what the function does.
On top of that
formData
is empty during the first iteration colliding with the actual method signature: [...]
Doesn't Record<string, boolean | number | string>
accept {}
?
There is not one test for this method.
Fair enough, although the test would be for objectToFormData
as it's the one that is exported.
I've spent a fair amount of time looking at it and still cannot get the grasp of it so it will be hard to maintain.
What's confusing about it? What would you suggest?
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.
I tried to give it a spin, but I did not manage to get rid of recursion in favor of reduce
and friends.
I don't think it makes sense to waste more time on this. It's a very specific case, and it currently works for the issue it is meant to solve.
If you have an alternative implementation - please share 😉
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.
I won't insist, I lost this battle. One last war cry in favor of SRP though: a function that does everything is wrong. A function must do one and only one thing. And it's OK to have a 1 or 2 line function as long as it complies with SRP. It makes it more readable and more maintainable.
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.
LOL, "function that does everything" was probably an overstatement 😅 IMO objectToFormData
actually does 1 thing (transform one object format to another), but it's self-contained, so you don't need to jump around the code to understand what it does.
But well, I understand your point also. Does this look better to you? a7a2e45
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.
Since the code is complicated, the first thing I did is try to make this work locally. I arranged everything (apisonator, etc) and tried.
It works fine and the backend registers the metrics, but I found a bug: it seems the number of hits registered by the backend is greater than I specified in the request. Please check the attached video, observe how the Hits
metric is 25
and after sending hits: 5
then Hits
is increased to 36
, it should be 25+5=30
, which leads me to think it's adding up also the 6
I set for to the hits.3
metric.
Screencast.from.2023-04-04.14-43-04.webm
const buildFormData = (formData: Record<string, boolean | number | string>, data: BodyValue, parentKey?: string) => { | ||
if (data && typeof data === 'object') { | ||
Object.keys(data).forEach((key: string) => { | ||
buildFormData(formData, data[key], parentKey ? `${parentKey}[${key}]` : key) | ||
}) | ||
} else { | ||
if (parentKey) { | ||
formData[parentKey] = data ? data : '' | ||
} | ||
} | ||
} |
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.
I think buildFormData
is OK to be inside objectToFormData
. Otherwise the only thing objectToFormData
would do is to call buildFormData
, no need to create a new function for that. In any case, what I would do is remove objectToFormData
and call buildFormData({}, body)
directly from transformReportRequestBody
.
5bd2238
to
986609e
Compare
Yes... that's the case. I don't like this behavior at all, and I raised an issue about that https://issues.redhat.com/browse/THREESCALE-7681 But as of today this behavior is expected. Thanks for taking time for verifying the end-to-end functionality! 🙌 |
Restore custom.d.ts Fixes Fix tests
fb0f801
to
ffa3f34
Compare
3dfe288
to
a7a2e45
Compare
Super weird, but OK. |
4b6c286
to
1f56654
Compare
This fixes the serialization issue with the
report
endpoint of the Service Management API, initially reported in this comment: #3103 (comment) (item 2).Actually, it serializes differently in different cases:
In order to fix this I considered several options:
Do it in
requestInterceptor
This was a bit messy, because at this point the request body is already serialized, and it would be tricky to parse, especially given that sometimes it's serialized as JSON, and other times as form-encoded string, and also could be mixed.
I also tried to write a plugin in the following way:
At first it looked promising, as
console.log(req)
was printing out therequestBody
in the browser console, however I then realized that at this point in the executionrequestBody
is actually not yet available, andconsole.log
was misleading.fn.execute
function of the SwaggerUI, which in its turn is just imported fromswagger-js
library.It's not ideal and quite "hacky", because it tweaks only the specific endpoint and in a very specific way.
Ideally, we could implement something more generic, that would serialize any object with config
deepObject
andexplode
in a way that we expect. Maybe, this would also potentially allow us to get rid of this hack of adding[]
to the parameters names, like here:porta/doc/active_docs/account_management_api.json
Lines 2524 to 2531 in eb07699
But I would leave this task for later, as an improvement.
For now, the serialization works as expected, the same way as in the original Swagger v1, with a slight difference on special characters encoding (e.g. spaces are encoded as
%20
rather than+
. But I think it applies to all APIs in general, and should be OK.Example:
Before
decoded body:
After
decoded body:
Before and after are exactly the same, except for the timestamp value:
Need to check if the new encoding actually works with apisonator, it should as
%20
is standard.The above bodies as screenshots in the UI:
Before
After (it's missing a
,
after the timestamp field, but I don't have time to re-screenshot now)Docs on SwaggerUI plugins: https://github.com/swagger-api/swagger-ui/blob/master/docs/customization/plugin-api.md