Skip to content

Commit

Permalink
Solve audit accessibility violation issue (#418)
Browse files Browse the repository at this point in the history 
* First draft of DocumentationConverter smart wrapping

* First draft of DocumentationConverter smart wrapping

* Add changelog to smart wrapping commit

* Squirrel out function of wrapping and writing service client docs

* Squirrel out function of wrapping and writing service client docs

* Modify changelog description

* First draft of DocumentationConverter smart wrapping

* First draft of DocumentationConverter smart wrapping

* Add changelog to smart wrapping commit

* Squirrel out function of wrapping and writing service client docs

* Squirrel out function of wrapping and writing service client docs

* Delete previous version's comment

* Optimize DocumentaionConverter write wrapped text logic

* modify changelog description

* Optimize DocumentaionConverter write wrapped text logic

* change remaining documentation converter and enum test code

* change enum test code

---------

Co-authored-by: Tianyi Wang <wty@amazon.com>
  • Loading branch information
@wty-Bryant
wty-Bryant and Tianyi Wang committed Apr 7, 2023
1 parent 6768320 commit 1521536
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 54 deletions.
8 changes: 8 additions & 0 deletions .changelog/1a8aa15f7f0649e3ba084e5ab48a6345.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id": "1a8aa15f-7f06-49e3-ba08-4e5ab48a6345",
"type": "bugfix",
"description": "correct list formatting in service client docs",
"modules": [
"codegen"
]
}
105 changes: 81 additions & 24 deletions ...hy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/DocumentationConverter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ private DocumentationConverter() {}
* @param docs commonmark formatted documentation
* @return godoc formatted documentation
*/
public static String convert(String docs) {
public static String convert(String docs, int docWrapLength) {
// Smithy's documentation format is commonmark, which can inline html. So here we convert
// to html so we have a single known format to work with.
String htmlDocs = HtmlRenderer.builder().escapeHtml(false).build().render(MARKDOWN_PARSER.parse(docs));
Expand All @@ -72,28 +72,35 @@ public static String convert(String docs) {
htmlDocs = Jsoup.clean(htmlDocs, GODOC_ALLOWLIST);

// Now we parse the html and visit the resultant nodes to render the godoc.
FormattingVisitor formatter = new FormattingVisitor();
FormattingVisitor formatter = new FormattingVisitor(docWrapLength);
Node body = Jsoup.parse(htmlDocs).body();
NodeTraversor.traverse(formatter, body);
return formatter.toString();
}

private static class FormattingVisitor implements NodeVisitor {
private static final Set<String> TEXT_BLOCK_NODES = SetUtils.of(
"br", "p", "h1", "h2", "h3", "h4", "h5", "h6"
"br", "p", "h1", "h2", "h3", "h4", "h5", "h6", "note"
);
private static final Set<String> LIST_BLOCK_NODES = SetUtils.of("ul", "ol");
private static final Set<String> CODE_BLOCK_NODES = SetUtils.of("pre", "code");
private final CodeWriter writer;

private boolean needsListPrefix = false;
private boolean shouldStripPrefixWhitespace = false;
private int docWrapLength;
private int listDepth;
// the last line string written, used to calculate the remaining spaces to reach docWrapLength
// and determine if a split char is needed between it and next string
private String lastLineString;

FormattingVisitor() {
FormattingVisitor(int docWrapLength) {
writer = new CodeWriter();
writer.trimTrailingSpaces(false);
writer.trimBlankLines();
writer.insertTrailingNewline(false);
this.docWrapLength = docWrapLength;
this.lastLineString = "";
}

@Override
Expand All @@ -109,7 +116,7 @@ public void head(Node node, int depth) {
writeNewline();
writeIndent();
} else if (LIST_BLOCK_NODES.contains(name)) {
writeNewline();
listDepth++;
} else if (name.equals("li")) {
// We don't actually write out the list prefix here in case the list element
// starts with one or more text blocks. By deferring writing those out until
Expand All @@ -119,33 +126,81 @@ public void head(Node node, int depth) {
}
}

private void writeNewline() {
// While jsoup will strip out redundant whitespace, it will still leave some. If we
// start a new line then we want to make sure we don't keep any prefixing whitespace.
shouldStripPrefixWhitespace = true;
writer.write("");
}

private void writeText(TextNode node) {
if (node.isBlank()) {
return;
}

// Docs can have valid $ characters that shouldn't run through formatters.
String text = node.text().replace("$", "$$");

if (shouldStripPrefixWhitespace) {
shouldStripPrefixWhitespace = false;
text = StringUtils.stripStart(text, " \t");
}

if (needsListPrefix) {
needsListPrefix = false;
writer.write("");
writeIndent();
text = "* " + StringUtils.stripStart(text, " \t");
if (listDepth > 0) {
text = StringUtils.stripStart(text, " \t");
if (needsListPrefix) {
needsListPrefix = false;
text = " - " + text;
writeNewline();
}

writeWrappedText(text, "\n ");
} else {
writeWrappedText(text, "\n");
}
}

private void writeWrappedText(String text, String newLineIndent) {
// check the last line's remaining space to see if test should be
// split to 2 parts to write to current and next line
// note that wrapped text will not contain desired indent at the beginning,
// so indent will be added to the wrapped text when it is written to a new line

// right boundary index of text to be written to the same line exceeding
// neither docWrapLength nor text length
int trailingLineCutoff = Math.min(Math.max(docWrapLength - lastLineString.length(), 0), text.length());
// the index of last space on the left of boundary if exist
int lastSpace = text.substring(0, trailingLineCutoff).lastIndexOf(" ");
// if current line is large enough to put the text, just append complete text to current line
// otherwise, cut out next line string starting from lastSpace index
String appendString = trailingLineCutoff < text.length() ? text.substring(0, lastSpace + 1) : text;
String nextLineString = trailingLineCutoff < text.length() ? text.substring(lastSpace + 1) : "";

if (!appendString.isEmpty()) {
ensureSplit(" ", appendString);
writeInline(appendString);
}
if (!nextLineString.isEmpty()) {
nextLineString = StringUtils.stripStart(newLineIndent, "\n")
+ StringUtils.wrap(nextLineString, docWrapLength, newLineIndent, false);
writeNewline();
writeInline(nextLineString);
}
}

private void ensureSplit(String split, String text) {
if (!text.startsWith(split) && !lastLineString.isEmpty() && !lastLineString.endsWith(split)) {
writeInline(split);
}
writer.writeInline(text);
}

private void writeNewline() {
// While jsoup will strip out redundant whitespace, it will still leave some. If we
// start a new line then we want to make sure we don't keep any prefixing whitespace.
// need to refresh last line string
shouldStripPrefixWhitespace = true;
writer.write("");
lastLineString = "";
}

private void writeInline(String contents, String... args) {
// write text at the current line, update last line string
String formatText = writer.format(contents, args);
writer.writeInlineWithNoFormatting(formatText);
formatText = lastLineString + formatText;
lastLineString = formatText.substring(formatText.lastIndexOf("\n") + 1);
}

void writeIndent() {
Expand Down Expand Up @@ -223,21 +278,23 @@ public void tail(Node node, int depth) {
writer.dedent();
}

if (TEXT_BLOCK_NODES.contains(name) || isTopLevelCodeBlock(node, depth)
|| LIST_BLOCK_NODES.contains(name)) {
writeNewline();
if (TEXT_BLOCK_NODES.contains(name) || isTopLevelCodeBlock(node, depth)) {
writeNewline();
} else if (LIST_BLOCK_NODES.contains(name)) {
listDepth--;
if (listDepth == 0) {
writeNewline();
}
} else if (name.equals("a")) {
String url = node.absUrl("href");
if (!url.isEmpty()) {
// godoc can't render links with text bodies, so we simply append the link.
// Full links do get rendered.
writer.writeInline(" ($L)", url);
writeInline(" ($L)", url);
}
} else if (name.equals("li")) {
// Clear out the expectation of a list element if the element's body is empty.
needsListPrefix = false;
writer.write("");
}
}

Expand Down
4 changes: 2 additions & 2 deletions codegen/smithy-go-codegen/src/main/java/software/amazon/smithy/go/codegen/GoWriter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -640,8 +640,8 @@ private void writeDocs(AbstractCodeWriter<GoWriter> writer, Runnable runnable) {
}

private void writeDocs(AbstractCodeWriter<GoWriter> writer, int docWrapLength, String docs) {
String wrappedDoc = StringUtils.wrap(DocumentationConverter.convert(docs), docWrapLength);
writeDocs(writer, () -> writer.write(wrappedDoc.replace("$", "$$")));
String convertedDoc = DocumentationConverter.convert(docs, docWrapLength);
writeDocs(writer, () -> writer.write(convertedDoc.replace("$", "$$")));
}

/**
Expand Down
56 changes: 28 additions & 28 deletions ...o-codegen/src/test/java/software/amazon/smithy/go/codegen/DocumentationConverterTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@
import org.junit.jupiter.params.provider.MethodSource;

public class DocumentationConverterTest {
private static final int DEFAULT_DOC_WRAP_LENGTH = 80;

@ParameterizedTest
@MethodSource("cases")
void convertsDocs(String given, String expected) {
assertThat(DocumentationConverter.convert(given), equalTo(expected));
assertThat(DocumentationConverter.convert(given, DEFAULT_DOC_WRAP_LENGTH), equalTo(expected));
}

private static Stream<Arguments> cases() {
Expand All @@ -36,35 +37,35 @@ private static Stream<Arguments> cases() {
),
Arguments.of(
"<ul><li>Testing 1 2 3</li> <li>FooBar</li></ul>",
"* Testing 1 2 3\n\n* FooBar"
" - Testing 1 2 3\n - FooBar"
),
Arguments.of(
"<ul> <li>Testing 1 2 3</li> <li>FooBar</li> </ul>",
"* Testing 1 2 3\n\n* FooBar"
" - Testing 1 2 3\n - FooBar"
),
Arguments.of(
" <ul> <li>Testing 1 2 3</li> <li>FooBar</li> </ul>",
"* Testing 1 2 3\n\n* FooBar"
" - Testing 1 2 3\n - FooBar"
),
Arguments.of(
"<ul> <li> <p>Testing 1 2 3</p> </li><li> <p>FooBar</p></li></ul>",
"* Testing 1 2 3\n\n* FooBar"
" - Testing 1 2 3\n - FooBar"
),
Arguments.of(
"<ul> <li><code>Testing</code>: 1 2 3</li> <li>FooBar</li> </ul>",
"* Testing: 1 2 3\n\n* FooBar"
" - Testing : 1 2 3\n - FooBar"
),
Arguments.of(
"<ul> <li><p><code>FOO</code> Bar</p></li><li><p><code>Xyz</code> ABC</p></li></ul>",
"* FOO Bar\n\n* Xyz ABC"
" - FOO Bar\n - Xyz ABC"
),
Arguments.of(
"<ul><li> foo</li><li>\tbar</li><li>\nbaz</li></ul>",
"* foo\n\n* bar\n\n* baz"
" - foo\n - bar\n - baz"
),
Arguments.of(
"<p><code>Testing</code>: 1 2 3</p>",
"Testing: 1 2 3"
"Testing : 1 2 3"
),
Arguments.of(
"<pre><code>Testing</code></pre>",
Expand All @@ -89,36 +90,35 @@ private static Stream<Arguments> cases() {
+ " configuration, see "
+ "<a href=\" https://docs.aws.amazon.com/AmazonS3/latest/dev/crr.html\">"
+ "Cross-Region Replication (CRR)</a> in the <i>Amazon S3 Developer Guide</i>. </p>",
"Deletes the replication configuration from the bucket. For information about replication "
+ "configuration, see Cross-Region Replication (CRR) "
+ "(https://docs.aws.amazon.com/AmazonS3/latest/dev/crr.html) in the Amazon S3 "
+ "Developer Guide."
"Deletes the replication configuration from the bucket. For information about\n" +
"replication configuration, see Cross-Region Replication (CRR) (https://docs.aws.amazon.com/AmazonS3/latest/dev/crr.html)\n" +
"in the Amazon S3 Developer Guide."
),
Arguments.of(
"* foo\n* bar",
"* foo\n\n* bar"
" - foo\n - bar"
),
Arguments.of(
"[a link](https://example.com)",
"a link (https://example.com)"
),
Arguments.of("", ""),
Arguments.of("<!-- foo bar -->", ""),
Arguments.of("# Foo\nbar", "Foo\n\nbar"),
Arguments.of("<h1>Foo</h1>bar", "Foo\n\nbar"),
Arguments.of("## Foo\nbar", "Foo\n\nbar"),
Arguments.of("<h2>Foo</h2>bar", "Foo\n\nbar"),
Arguments.of("### Foo\nbar", "Foo\n\nbar"),
Arguments.of("<h3>Foo</h3>bar", "Foo\n\nbar"),
Arguments.of("#### Foo\nbar", "Foo\n\nbar"),
Arguments.of("<h4>Foo</h4>bar", "Foo\n\nbar"),
Arguments.of("##### Foo\nbar", "Foo\n\nbar"),
Arguments.of("<h5>Foo</h5>bar", "Foo\n\nbar"),
Arguments.of("###### Foo\nbar", "Foo\n\nbar"),
Arguments.of("<h6>Foo</h6>bar", "Foo\n\nbar"),
Arguments.of("# Foo\nbar", "Foo\nbar"),
Arguments.of("<h1>Foo</h1>bar", "Foo\nbar"),
Arguments.of("## Foo\nbar", "Foo\nbar"),
Arguments.of("<h2>Foo</h2>bar", "Foo\nbar"),
Arguments.of("### Foo\nbar", "Foo\nbar"),
Arguments.of("<h3>Foo</h3>bar", "Foo\nbar"),
Arguments.of("#### Foo\nbar", "Foo\nbar"),
Arguments.of("<h4>Foo</h4>bar", "Foo\nbar"),
Arguments.of("##### Foo\nbar", "Foo\nbar"),
Arguments.of("<h5>Foo</h5>bar", "Foo\nbar"),
Arguments.of("###### Foo\nbar", "Foo\nbar"),
Arguments.of("<h6>Foo</h6>bar", "Foo\nbar"),
Arguments.of("Inline `code`", "Inline code"),
Arguments.of("```\ncode block\n```", " code block"),
Arguments.of("```java\ncode block\n```", " code block"),
Arguments.of("```\ncode block\n ```", " code block"),
Arguments.of("```java\ncode block\n ```", " code block"),
Arguments.of("foo<br/>bar", "foo\n\nbar"),
Arguments.of(" <p>foo</p>", "foo")
);
Expand Down

0 comments on commit 1521536

Please sign in to comment.