-
Notifications
You must be signed in to change notification settings - Fork 54
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
Normalize Slack form element presentational data | Attempt 2 #258
Conversation
f86d507
to
e923f50
Compare
return SlackFormTextElement.builder() | ||
.from(element) | ||
.setPlaceholder(normalizePlaceholder(element)) | ||
.setLabel(normalizeLabel(element)) | ||
.setHint(normalizeHint(element)) | ||
.build(); |
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.
Replaced with
calls with builder here.
return SlackFormTextareaElement.builder() | ||
.from(element) | ||
.setPlaceholder(normalizePlaceholder(element)) | ||
.setLabel(normalizeLabel(element)) | ||
.setHint(normalizeHint(element)) | ||
.build(); |
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.
Replaced with
calls with builder here.
return SlackFormSelectElement.builder() | ||
.from(element) | ||
.setPlaceholder(normalizePlaceholder(element)) | ||
.setLabel(normalizeLabel(element)) | ||
.setOptionGroups(normalizeOptionGroups(element)) | ||
.setOptions(normalizeOptions(element)) | ||
.build(); |
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.
Replaced with
calls with builder here.
if (shouldNormalizePlaceholder(element) | ||
|| shouldNormalizeLabel(element, SlackDialogFormElementLengthLimits.MAX_LABEL_LENGTH) | ||
|| shouldNormalize(element.getOptionGroups(), SlackDialogFormElementLengthLimits.MAX_OPTION_GROUPS_NUMBER) | ||
|| shouldNormalizeOptions(element)) { | ||
return SlackFormSelectElement.builder() | ||
.from(element) | ||
.setPlaceholder(normalizePlaceholder(element)) | ||
.setLabel(normalizeLabel(element)) | ||
.setOptionGroups(normalizeOptionGroups(element)) | ||
.setOptions(normalizeOptions(element)) | ||
.build(); |
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'll describe the stack overflow problem with an example. Let's say we have the old code here:
if (shouldNormalizePlaceholder(element)
|| shouldNormalizeLabel(element, SlackDialogFormElementLengthLimits.MAX_LABEL_LENGTH)
|| shouldNormalize(element.getHint(), SlackDialogFormElementLengthLimits.MAX_HINT_LENGTH)) {
return SlackFormTextElement.copyOf(element)
.withPlaceholder(normalizePlaceholder(element))
.withLabel(normalizeLabel(element))
.withHint(normalizeHint(element));
}
return element;
What we do here is define whether we should normalize any data first and then normalize it. The problem is actually obvious but still tricky. Let's say we have a very long label
that should be normalized. In this case, when SlackFormSelectElement
is created we do next:
- Check if we should normalize any field.
- Determine that we should normalize some fields(
label
). - Start normalization process:
- Normalize placeholer and then call
withPlaceholder
. - Normalize label and then call
withLabel
. - Normalize the rest.
- Build the final normalized object.
- Normalize placeholer and then call
The flow I described above is what I think is happening. What was actually happening is we were entering an endless loop upon calling withPlaceholder
because unlike builder, this method creates the actual SlackFormSelectElement
so it calls method annotated with @Value.Check
, which then initiates the normalization process once again. SInce we didn't normalize the label yet, we determine that label
still has to be normalized and run SlackFormTextElement.copyOf(element).withPlaceholder(normalizePlaceholder(element)).withLabel(normalizeLabel(element))
once again, which results in doing this again, and then again, and again.
This issue wasn't caught at first because it depends on the order of the checks and normalization as well as on the data to be normalized. If we place withLabel
before withPlaceholder
and try to build the object with a long label, everything works fine because on the call to withLabel
method we have a fully normalized object that passes all the checks.
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.
LGTM!
Same as #256 but fixed stackoverflow error during normalization caused by cyclic constructor calls in
with
methods.