-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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 WFS property format writer for multiline strings #15638
base: main
Are you sure you want to change the base?
Fix WFS property format writer for multiline strings #15638
Conversation
📦 Preview the website for this branch here: https://deploy-preview-15638--ol-site.netlify.app/. |
41be1a5
to
6936d9a
Compare
src/ol/format/WFS.js
Outdated
@@ -938,7 +939,11 @@ function writeProperty(node, pair, objectStack) { | |||
GML32.prototype.writeGeometryElement(value, pair.value, objectStack); | |||
} | |||
} else { | |||
writeStringTextNode(value, pair.value); | |||
if (typeof pair.value === 'string' && pair.value.search('\n') >= 0) { |
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.
if (typeof pair.value === 'string' && pair.value.search('\n') >= 0) { | |
if (typeof pair.value === 'string' && pair.value.includes('\n') >= 0) { |
The above should work as well, and yield better performance because no regular expression is created.
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.
Did you mean to remove the >= 0
part as well, @ahocevar, like below?
if (typeof pair.value === 'string' && pair.value.search('\n') >= 0) { | |
if (typeof pair.value === 'string' && pair.value.includes('\n')) { |
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.
Yes, thanks @rdewit
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.
When looking at the issue more closely, I wonder if it wouldn't be better to completely skip the test and always output a CDATA block.
Indeed, it is not only end of lines that are not correctly interpreted in a text node: any white space combination (space, tab, end of line) is interpreted as a single space (as per the html/xml rules), so that for example a string with multiple consecutive spaces is also not correctly handled.
But using CDATA blocks are not the perfect solution either: they cannot contain the end of block sequence ("]]>").
They also cannot contain arbitrary binary data (it must be a valid utf8 string) but this is probably not a problem since WFS property values are supposed to be valid xml text (but I don't know if the WF specs explicitly says something about this)
For simple use cases (multi-line string, which I needed initially), CDATA blocks are working great out of the box with geoserver (and any xml parser), but maybe the correct solution for the general case would be to have an API to let the user handle the serialization himself (base64 ?)
What do you think?
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 following may be the best compromise to catch any white space combination without using CDATA blocks for single words:
[ ' ', '\n', '\t' ].some(c => pair.value.includes(c)
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.
Fine with me to always create a CDATA block.
6936d9a
to
97fef06
Compare
If you have text with pair.value.split(']]>').forEach((part, i, a) => {
if (i < a.length - 1) {
part += ']]';
}
if (i > 0) {
part = '>' + part;
}
writeCDATASection(value, part);
}); |
97fef06
to
3c13544
Compare
src/ol/format/WFS.js
Outdated
try { | ||
// Generate a CDATA section to preserve whitespaces. | ||
writeCDATASection(value, pair.value); | ||
} catch { | ||
// If value contains the CDATA closing marker ']]>', fallback to a text node. | ||
writeStringTextNode(value, pair.value); | ||
} |
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.
Not sure if it would work, but how about
try { | |
// Generate a CDATA section to preserve whitespaces. | |
writeCDATASection(value, pair.value); | |
} catch { | |
// If value contains the CDATA closing marker ']]>', fallback to a text node. | |
writeStringTextNode(value, pair.value); | |
} | |
pair.value.split(']]>').forEach((text) => writeCDataSection(value, text)); |
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.
Sorry I thought I already made the change following @M393 's remark but failed to push correctly.
Should be good now.
When updating a feature through WFS, if there are any newline characters in a string properties, these are lost (for example, geoserver handles these newlines as whitespace). To fix this, multiline string properties should be encoded as CDATA block.
3c13544
to
5fc0e90
Compare
if (i > 0) { | ||
part = '>' + part; | ||
} | ||
writeCDATASection(value, part); |
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 this addition could benefit from a test case.
When updating a feature through WFS, if there are any newline
characters in a string properties, these are lost (for example,
geoserver handles these newlines as whitespace).
To fix this, multiline string properties should be encoded as
CDATA block.
Fix #10127