Sort output config fields stably#4746
Conversation
|
|
|
Unfortunately I seem not to have access to your Jenkins so I can't even figure out my mistake. |
|
As discussed w/ @jalogisch (offline):
|
edmundoa
left a comment
edmundoa
left a comment
There was a problem hiding this comment.
Other than the inline comments, I'm not sure how reliable this approach is across browsers.
On the backend, we use a LinkedHashMap that will guarantee order, but on the frontend we store the results in an Object, and Object properties are not guaranteed to be ordered, even if most browsers do.
If we want to guarantee to keep fields in order, we should use another strategy, for instance sorting by optionality and then by key name. That will at least guarantee that all fields are in the same order in all browsers, as long as key names are not changed.
|
|
||
| const configFieldKeys = $.map(this.state.configFields, (v, k) => k).sort(this._sortByOptionality); | ||
| const configFieldKeys = $.map(this.state.configFields, (v, k) => k) | ||
| .map((v, i) => ({ key: v, pos: i })) |
There was a problem hiding this comment.
I know you didn't wrote that line, but I would vote for replacing the $.map(this.state.configFields, (v, k) => k) with the much clearer, in my opinion, Object.keys(this.state.configFields).
Please also use a clearer name for v. I suggest key instead. In the end we minify the code, so better make it easier to understand.
|
|
||
| _sortByOptionality(x1, x2) { | ||
| return (this.state.configFields[x1].is_optional - this.state.configFields[x2].is_optional); | ||
| return this.state.configFields[x1.key].is_optional - this.state.configFields[x2.key].is_optional || x1.pos - x2.pos; |
There was a problem hiding this comment.
I think we should rewrite this in a more explicit way.
Subtracting two booleans is already hard to understand without deep understanding of JS internals, and using the or operator to do another comparison when the previous result was 0 only makes it harder to understand.
|
I got carried on by the review and I forgot to mention: thank you very much for your contribution! Let's work through the code and get this fix for good 🙂 |
|
Hello @edmundoa and thank you for responding!
This would be a breaking change for our output plugin and for all the others. Besides that.. AFAIK no one complained about that. So I won't change that – at least not in this PR as it doesn't solve the problem. But you can expect the other changes soon. Best, |
|
@Al2Klimov sounds reasonable to me. For what I read, most browsers respect the To further guarantee that the inputs will appear in the order specified by the author (even if it is implicitly), we would need to explicitly export the order of each key in the API, allowing to read that property from the JS code and sorting by it. That change would be fine for 3.0, but it would be a no-go for 2.4.x. |
|
@Al2Klimov to be honest, I don't know the reason for it and it probably happened organically. I guess the reason was to let users see the mandatory fields they had to fill out first, in case they didn't pay much attention to the form. Also sometimes configuration fields are put together from different classes and maybe the sorting looked somehow "wrong". Feel free to open an issue for that so we can discuss it and maybe also change it. |
|
Thank you for your contribution @Al2Klimov! Do you want to update the 2.4 branch with the changes or should I? |
|
I'll do. |
3 participants
Description
fixes #4745
Motivation and Context
#4745
How Has This Been Tested?
Not at all. Unfortunately I couldn't build Graylog independent of my change.
Screenshots (if appropriate):
N/A
Types of changes
Checklist: