diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e0571e..e419630 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -58,6 +58,7 @@ format changes. ### Fixed +- **`Format.formatDocument` keeps every directive in document order** (#188; trendvidia/protowire#558). It used to drop `@proto`, and wrote every named directive before every dataset, so `@dataset t.v1.T (id)` / `(1)` followed by `@entry x { a = 1 }` came out with the `@entry` first. That order matters: an anonymous `@proto` binds the typed directive after it. `@type` is still written first, since it binds no `@proto`. The other directives follow in source order, each after its comments, with no blank line between them; a dataset with no type is written `@dataset (id)`. A document whose only directive is `@type` formats exactly as before. The output is byte for byte protowire-go's `FormatDocument` (protowire-go#262). - **`Convert.xmlToProto` keeps the length of a `uint8` or `int8` array** (#179). `` imported as one `uint32` with `(sbe.encoding) = "uint8"`, so a 16-byte field was read as 1 byte and every later field in the block was misplaced. It now imports as `bytes` with `(sbe.length) = 16`, the same 16 bytes on the wire. - **The SBE decoder rejects a string field that is not UTF-8** (#162; `docs/HARDENING.md` § UTF-8). Both tiers turned the bytes of an `(sbe.length)` `string` into a `String` with `new String(bytes, UTF_8)`, which puts U+FFFD in place of bad bytes and says nothing, so `41 42 FF FE` decoded as `"AB\uFFFD\uFFFD"`. `SbeWireCodec` and `View.stringValue` now decode strictly and throw `IllegalArgumentException` (`sbe: invalid UTF-8 in string field `). A `bytes` field takes any bytes, as before. `check-decode-android` decodes `adversarial.v1.SbeText`, so the lite tier runs the spec's `sbe/utf8-*` rows. diff --git a/pxf-runtime/src/main/java/org/protowire/pxf/Format.java b/pxf-runtime/src/main/java/org/protowire/pxf/Format.java index 7cc2dd0..3d25693 100644 --- a/pxf-runtime/src/main/java/org/protowire/pxf/Format.java +++ b/pxf-runtime/src/main/java/org/protowire/pxf/Format.java @@ -6,7 +6,9 @@ import java.math.MathContext; import java.math.RoundingMode; import java.nio.charset.StandardCharsets; +import java.util.ArrayList; import java.util.Base64; +import java.util.Comparator; import java.util.List; import java.util.function.Predicate; @@ -14,25 +16,84 @@ public final class Format { private Format() {} + /** + * Pretty-prints a document, preserving comments. + * + *

It keeps every directive, with its dataset rows, in document order + * (trendvidia/protowire#558, #188). {@code @type} is written first: it + * binds no anonymous {@code @proto}, so moving it changes nothing. The + * other directives follow in source order, each after its comments: a + * named directive's body and a {@code @proto} body as written, and a + * dataset as its header and then one row per line, an absent cell left + * empty. A document whose only directive is {@code @type} formats as it + * always has. The output is byte for byte protowire-go's + * {@code FormatDocument}. + */ public static String formatDocument(Ast.Document doc) { StringBuilder sb = new StringBuilder(); + List others = topDirectives(doc); + if (others.isEmpty()) { + if (!doc.typeUrl().isEmpty()) { + sb.append("@type ").append(doc.typeUrl()).append("\n\n"); + } + writeComments(sb, doc.leadingComments(), 0); + formatEntries(sb, doc.entries(), 0); + return sb.toString(); + } + if (!doc.typeUrl().isEmpty()) { - sb.append("@type ").append(doc.typeUrl()).append("\n\n"); + sb.append("@type ").append(doc.typeUrl()).append('\n'); } - for (Ast.Directive d : doc.directives()) { - formatDirective(sb, d); - sb.append('\n'); + writeComments(sb, doc.leadingComments(), 0); + for (Object o : others) { + switch (o) { + case Ast.Directive d -> { + writeComments(sb, d.leadingComments(), 0); + formatNamedDirective(sb, d); + } + case Ast.DatasetDirective d -> { + writeComments(sb, d.leadingComments(), 0); + formatDataset(sb, d); + } + case Ast.ProtoDirective d -> { + writeComments(sb, d.leadingComments(), 0); + formatProto(sb, d); + } + default -> throw new IllegalStateException( + "Format: unexpected directive type " + o.getClass().getSimpleName()); + } } - for (Ast.DatasetDirective t : doc.datasets()) { - formatTableDirective(sb, t); + if (!doc.entries().isEmpty()) { sb.append('\n'); + formatEntries(sb, doc.entries(), 0); } - writeComments(sb, doc.leadingComments(), 0); - formatEntries(sb, doc.entries(), 0); return sb.toString(); } - private static void formatDirective(StringBuilder sb, Ast.Directive d) { + /** + * The document's directives other than {@code @type}, which the + * {@link Ast.Document} keeps in three lists, merged back into source + * order: an anonymous {@code @proto} binds the typed directive after it, + * so the order is part of the meaning. Each element is an + * {@link Ast.Directive}, {@link Ast.DatasetDirective} or + * {@link Ast.ProtoDirective}. + */ + private static List topDirectives(Ast.Document doc) { + record At(int offset, Object d) {} + List all = new ArrayList<>(); + for (Ast.Directive d : doc.directives()) all.add(new At(d.pos().offset(), d)); + for (Ast.DatasetDirective d : doc.datasets()) all.add(new At(d.pos().offset(), d)); + for (Ast.ProtoDirective d : doc.protos()) all.add(new At(d.pos().offset(), d)); + // List.sort is stable, so directives at one offset (built in code, + // with no position) keep the order of the lists above. + all.sort(Comparator.comparingInt(At::offset)); + List out = new ArrayList<>(all.size()); + for (At a : all) out.add(a.d()); + return out; + } + + /** Writes {@code @ prefix* [{body}]}, the body as it was written. */ + private static void formatNamedDirective(StringBuilder sb, Ast.Directive d) { sb.append('@').append(d.name()); for (String p : d.prefixes()) { sb.append(' ').append(p); @@ -43,25 +104,35 @@ private static void formatDirective(StringBuilder sb, Ast.Directive d) { sb.append('\n'); } - private static void formatTableDirective(StringBuilder sb, Ast.DatasetDirective t) { - sb.append("@dataset ").append(t.type()).append(" ("); - for (int i = 0; i < t.columns().size(); i++) { - if (i > 0) sb.append(", "); - sb.append(t.columns().get(i)); - } - sb.append(")\n"); + /** Writes the header, then one row per line. A null cell is an absent field, left empty; {@code null} is written. */ + private static void formatDataset(StringBuilder sb, Ast.DatasetDirective t) { + sb.append("@dataset"); + if (!t.type().isEmpty()) sb.append(' ').append(t.type()); + sb.append(" (").append(String.join(", ", t.columns())).append(")\n"); for (Ast.DatasetRow row : t.rows()) { sb.append('('); for (int i = 0; i < row.cells().size(); i++) { if (i > 0) sb.append(", "); Ast.Value cell = row.cells().get(i); if (cell != null) formatValue(sb, cell, 0); - // null cell ⇒ empty between commas, leave it blank. } sb.append(")\n"); } } + /** Writes a {@code @proto} directive in the shape it was read in. */ + private static void formatProto(StringBuilder sb, Ast.ProtoDirective d) { + sb.append("@proto"); + switch (d.shape()) { + case ANONYMOUS -> sb.append(" {").append(new String(d.body(), StandardCharsets.UTF_8)).append('}'); + case NAMED -> sb.append(' ').append(d.typeName()) + .append(" {").append(new String(d.body(), StandardCharsets.UTF_8)).append('}'); + case SOURCE -> sb.append(" \"\"\"").append(new String(d.body(), StandardCharsets.UTF_8)).append("\"\"\""); + case DESCRIPTOR -> sb.append(" b\"").append(Base64.getEncoder().encodeToString(d.body())).append('"'); + } + sb.append('\n'); + } + private static void writeIndent(StringBuilder sb, int level) { for (int i = 0; i < level; i++) sb.append(" "); } diff --git a/pxf-runtime/src/test/java/org/protowire/pxf/FormatDirectivesTest.java b/pxf-runtime/src/test/java/org/protowire/pxf/FormatDirectivesTest.java new file mode 100644 index 0000000..b07140f --- /dev/null +++ b/pxf-runtime/src/test/java/org/protowire/pxf/FormatDirectivesTest.java @@ -0,0 +1,155 @@ +// SPDX-License-Identifier: MIT +// Copyright (c) 2026 TrendVidia, LLC. +package org.protowire.pxf; + +import org.junit.jupiter.api.Test; + +import java.nio.charset.StandardCharsets; +import java.util.ArrayList; +import java.util.List; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * {@link Format#formatDocument} keeps every directive, with its dataset + * rows, in document order (trendvidia/protowire#558, #188). A port of + * protowire-go's {@code format_directives_test.go}; the expected texts are + * the ones protowire-go's {@code FormatDocument} writes for the same sources. + */ +class FormatDirectivesTest { + + /** + * Formats {@code src}, checks that the output parses to the same + * directives and is a fixed point of formatting, and returns it. + */ + private static String formatRoundTrip(String src) { + Ast.Document doc = Parser.parse(src); + String out = Format.formatDocument(doc); + Ast.Document again = Parser.parse(out); + assertEquals(directiveShape(doc), directiveShape(again), out); + assertEquals(out, Format.formatDocument(again), "formatting is a fixed point"); + return out; + } + + /** What a document's directives say, without positions or comments. */ + private static List directiveShape(Ast.Document doc) { + List out = new ArrayList<>(); + out.add(doc.typeUrl()); + for (Ast.Directive d : doc.directives()) { + out.add("@" + d.name()); + out.add(d.prefixes()); + out.add(d.body() == null ? null : new String(d.body(), StandardCharsets.UTF_8)); + } + for (Ast.DatasetDirective d : doc.datasets()) { + out.add("@dataset"); + out.add(d.type()); + out.add(d.columns()); + for (Ast.DatasetRow r : d.rows()) { + List cells = new ArrayList<>(); + for (Ast.Value c : r.cells()) { + if (c == null) { + cells.add(""); + } else { + StringBuilder sb = new StringBuilder(); + Format.formatValue(sb, c, 0); + cells.add(sb.toString()); + } + } + out.add(cells); + } + } + for (Ast.ProtoDirective d : doc.protos()) { + out.add("@proto"); + out.add(d.shape()); + out.add(d.typeName()); + out.add(new String(d.body(), StandardCharsets.UTF_8)); + } + out.add(doc.entries().size()); + return out; + } + + /** The order the directives come in, @type aside: what the shape above leaves out. */ + private static List directiveOrder(Ast.Document doc) { + List out = new ArrayList<>(); + for (Ast.Directive d : doc.directives()) out.add(d.pos().offset() + " @" + d.name()); + for (Ast.DatasetDirective d : doc.datasets()) out.add(d.pos().offset() + " @dataset"); + for (Ast.ProtoDirective d : doc.protos()) out.add(d.pos().offset() + " @proto"); + out.sort((a, b) -> Integer.compare( + Integer.parseInt(a.substring(0, a.indexOf(' '))), Integer.parseInt(b.substring(0, b.indexOf(' '))))); + return out.stream().map(s -> s.substring(s.indexOf(' ') + 1)).toList(); + } + + @Test + void dataset() { + String out = formatRoundTrip("@dataset trades.v1.Trade (id, px)\n(1, 2.5)\n(2, 3.5)\n"); + assertEquals("@dataset trades.v1.Trade (id, px)\n(1, 2.5)\n(2, 3.5)\n", out); + + // An empty cell is an absent field and null a present one; both stay. + out = formatRoundTrip("@dataset trades.v1.Trade (id, px, note)\n(1, 2.5, )\n(2, , null)\n"); + assertEquals("@dataset trades.v1.Trade (id, px, note)\n(1, 2.5, )\n(2, , null)\n", out); + } + + @Test + void entryAndNamedDirective() { + String src = "@type a.v1.M\n@entry x { a = 1 }\n@header h.v1.H\n\nname = \"n\"\n"; + assertEquals(src, formatRoundTrip(src)); + } + + @Test + void protoShapes() { + String[][] cases = { + {"@proto { string id = 1; }\n@dataset (id)\n(\"a\")\n", null}, + {"@proto a.v1.R { string id = 1; }\n@dataset a.v1.R (id)\n(\"a\")\n", null}, + {"@proto \"\"\"syntax = \"proto3\"; message R { string id = 1; }\"\"\"\n@dataset R (id)\n(\"a\")\n", null}, + // @type moves to the front: it binds no @proto, so this changes nothing. + {"@proto b\"CgA=\"\n@type a.v1.M\n\nx = 1\n", "@type a.v1.M\n@proto b\"CgA=\"\n\nx = 1\n"}, + }; + for (String[] c : cases) { + String want = c[1] == null ? c[0] : c[1]; + assertEquals(want, formatRoundTrip(c[0]), c[0]); + } + } + + /** + * An anonymous @proto binds the next typed directive, so a @proto + * written after a dataset in the source stays after it. + */ + @Test + void directivesKeepSourceOrder() { + String src = "@dataset t.v1.T (id)\n(1)\n@proto { string id = 1; }\n@dataset (id)\n(\"a\")\n"; + assertEquals(src, formatRoundTrip(src)); + } + + @Test + void directiveComments() { + String out = formatRoundTrip("# about the rows\n@dataset t.v1.T (id)\n(1)\n# second\n@dataset t.v1.U (id)\n(2)\n"); + assertTrue(out.contains("# about the rows\n@dataset t.v1.T (id)"), out); + assertTrue(out.contains("# second\n@dataset t.v1.U (id)"), out); + } + + /** A document whose only directive is @type formats as it always has. */ + @Test + void onlyTypeUnchanged() { + Ast.Document doc = Parser.parse("# c\n@type a.v1.M\n\nx = 1\n"); + assertEquals("@type a.v1.M\n\n# c\nx = 1\n", Format.formatDocument(doc)); + } + + /** #188, first input: a @proto before an @entry used to be dropped. */ + @Test + void issue188ProtoIsKept() { + String src = "@proto a.v1.P { int32 b = 1; }\n@entry x { a = 1 }\n"; + String out = formatRoundTrip(src); + assertEquals(src, out); + assertEquals(List.of("@proto", "@entry"), directiveOrder(Parser.parse(out))); + } + + /** #188, second input: an @entry after a dataset used to be written before it. */ + @Test + void issue188DatasetBeforeEntryKeepsOrder() { + String src = "@dataset t.v1.T (id)\n(1)\n@entry x { a = 1 }\n"; + String out = formatRoundTrip(src); + assertEquals(src, out); + assertEquals(List.of("@dataset", "@entry"), directiveOrder(Parser.parse(out))); + } +}