Restores the previous binary reader implementation's local symbol tab…

…le handling behavior for several edge cases.
amazon-ion · Oct 20, 2023 · 9a74c6a · 9a74c6a
1 parent 43296c7
commit 9a74c6a
Show file tree

Hide file tree

Showing 9 changed files with 147 additions and 22 deletions.
diff --git a/src/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java b/src/com/amazon/ion/impl/IonReaderContinuableApplicationBinary.java
@@ -214,7 +214,7 @@ private SymbolTable getSystemSymbolTable() {
     /**
      * Read-only snapshot of the local symbol table at the reader's current position.
      */
-    private class LocalSymbolTableSnapshot implements SymbolTable, SymbolTableAsStruct {
+    private class LocalSymbolTableSnapshot implements _Private_LocalSymbolTable, SymbolTableAsStruct {
 
         // The system symbol table.
         private final SymbolTable system = IonReaderContinuableApplicationBinary.this.getSystemSymbolTable();
@@ -425,6 +425,17 @@ public IonStruct getIonRepresentation(ValueFactory valueFactory) {
             }
             return structCache.getIonRepresentation(valueFactory);
         }
+
+        @Override
+        public _Private_LocalSymbolTable makeCopy() {
+            // This is a mutable copy. LocalSymbolTable handles the mutability concerns.
+            return new LocalSymbolTable(importedTables, Arrays.asList(idToText));
+        }
+
+        @Override
+        public SymbolTable[] getImportedTablesNoCopy() {
+            return importedTables.getImportedTablesNoCopy();
+        }
     }
 
     /**
@@ -544,7 +555,7 @@ String getSymbol(int sid) {
         }
         int localSymbolOffset = sid - firstLocalSymbolId;
         if (localSymbolOffset > localSymbolMaxOffset) {
-            throw new IonException("Symbol ID exceeds the max ID of the symbol table.");
+            throw new UnknownSymbolException(sid);
         }
         return symbols[localSymbolOffset];
     }
@@ -565,7 +576,7 @@ private SymbolToken getSymbolToken(int sid) {
             }
         }
         if (sid >= symbolTableSize) {
-            throw new IonException("Symbol ID exceeds the max ID of the symbol table.");
+            throw new UnknownSymbolException(sid);
         }
         SymbolToken token = symbolTokensById.get(sid);
         if (token == null) {

diff --git a/src/com/amazon/ion/impl/IonReaderContinuableTopLevelBinary.java b/src/com/amazon/ion/impl/IonReaderContinuableTopLevelBinary.java
@@ -97,6 +97,11 @@ public SymbolTable pop_passed_symbol_table() {
             return null;
         }
         symbolTableLastTransferred = currentSymbolTable;
+        if (symbolTableLastTransferred.isLocalTable()) {
+            // This method is called when transferring the reader's symbol table to either a writer or an IonDatagram.
+            // Those cases require a mutable copy of the reader's symbol table.
+            return ((_Private_LocalSymbolTable) symbolTableLastTransferred).makeCopy();
+        }
         return symbolTableLastTransferred;
     }
 

diff --git a/src/com/amazon/ion/impl/LocalSymbolTable.java b/src/com/amazon/ion/impl/LocalSymbolTable.java
@@ -51,7 +51,7 @@
  * Instances of this class are safe for use by multiple threads.
  */
 class LocalSymbolTable
-    implements SymbolTable
+    implements _Private_LocalSymbolTable
 {
 
     static class Factory implements _Private_LocalSymbolTableFactory
@@ -329,7 +329,8 @@ else if (fieldType == IonType.SYMBOL && ION_SYMBOL_TABLE.equals(reader.stringVal
         return new LocalSymbolTableImports(importsList);
     }
 
-    synchronized LocalSymbolTable makeCopy()
+    @Override
+    public synchronized _Private_LocalSymbolTable makeCopy()
     {
         return new LocalSymbolTable(this, getMaxId());
     }
@@ -604,19 +605,8 @@ public SymbolTable[] getImportedTables()
         return myImportsList.getImportedTables();
     }
 
-    /**
-     * Returns the imported symbol tables without making a copy.
-     * <p>
-     * <b>Note:</b> Callers must not modify the resulting SymbolTable array!
-     * This will violate the immutability property of this class.
-     *
-     * @return
-     *          the imported symtabs, as-is; the first element is a system
-     *          symtab, the rest are non-system shared symtabs
-     *
-     * @see #getImportedTables()
-     */
-    SymbolTable[] getImportedTablesNoCopy()
+    @Override
+    public SymbolTable[] getImportedTablesNoCopy()
     {
         return myImportsList.getImportedTablesNoCopy();
     }

diff --git a/src/com/amazon/ion/impl/_Private_IonBinaryWriterBuilder.java b/src/com/amazon/ion/impl/_Private_IonBinaryWriterBuilder.java
@@ -157,7 +157,7 @@ public void setInitialSymbolTable(SymbolTable symtab)
             if (symtab.isLocalTable())
             {
                 SymbolTable[] imports =
-                    ((LocalSymbolTable) symtab).getImportedTablesNoCopy();
+                    ((_Private_LocalSymbolTable) symtab).getImportedTablesNoCopy();
                 for (SymbolTable imported : imports)
                 {
                     if (imported.isSubstitute())
@@ -353,7 +353,7 @@ SymbolTable buildContextSymbolTable()
             return myInitialSymbolTable;
         }
 
-        return ((LocalSymbolTable) myInitialSymbolTable).makeCopy();
+        return ((_Private_LocalSymbolTable) myInitialSymbolTable).makeCopy();
     }
 
 

diff --git a/src/com/amazon/ion/impl/_Private_LocalSymbolTable.java b/src/com/amazon/ion/impl/_Private_LocalSymbolTable.java
@@ -0,0 +1,25 @@
+package com.amazon.ion.impl;
+
+import com.amazon.ion.SymbolTable;
+
+interface _Private_LocalSymbolTable extends SymbolTable {
+
+    /**
+     * @return a mutable copy of the symbol table.
+     */
+    _Private_LocalSymbolTable makeCopy();
+
+    /**
+     * Returns the imported symbol tables without making a copy.
+     * <p>
+     * <b>Note:</b> Callers must not modify the resulting SymbolTable array!
+     * This will violate the immutability property of this class.
+     *
+     * @return
+     *          the imported symtabs, as-is; the first element is a system
+     *          symtab, the rest are non-system shared symtabs
+     *
+     * @see SymbolTable#getImportedTables()
+     */
+    SymbolTable[] getImportedTablesNoCopy();
+}
diff --git a/src/com/amazon/ion/impl/_Private_Utils.java b/src/com/amazon/ion/impl/_Private_Utils.java
@@ -800,7 +800,7 @@ public static SymbolTable copyLocalSymbolTable(SymbolTable symtab)
         }
 
         SymbolTable[] imports =
-            ((LocalSymbolTable) symtab).getImportedTablesNoCopy();
+            ((_Private_LocalSymbolTable) symtab).getImportedTablesNoCopy();
 
         // Iterate over each import, we assume that the list of imports
         // rarely exceeds 5.
@@ -816,7 +816,7 @@ public static SymbolTable copyLocalSymbolTable(SymbolTable symtab)
             }
         }
 
-        return ((LocalSymbolTable) symtab).makeCopy();
+        return ((_Private_LocalSymbolTable) symtab).makeCopy();
     }
 
     /**

diff --git a/test/com/amazon/ion/DatagramTest.java b/test/com/amazon/ion/DatagramTest.java
@@ -28,8 +28,11 @@
 import com.amazon.ion.impl._Private_IonValue;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.nio.charset.StandardCharsets;
 import java.util.Collection;
 import java.util.Iterator;
+
+import com.amazon.ion.system.IonReaderBuilder;
 import org.junit.Before;
 import org.junit.Test;
 
@@ -720,4 +723,25 @@ public void testSetWithSymbolTable()
         assertEquals(yID + 1, v1.symbolValue().getSid()); //now x has a mapping in the existing symbol table
 
     }
+
+    @Test
+    public void modifySymbolsAfterLoadThenSerialize()
+        throws Exception
+    {
+        IonDatagram dg = system().getLoader().load(TestUtils.ensureBinary(system(), "{foo: bar::baz}".getBytes(StandardCharsets.UTF_8)));
+        IonStruct struct = (IonStruct) dg.get(0);
+        IonSymbol baz = (IonSymbol) struct.get("foo");
+        baz.setTypeAnnotations("bar", "abc");
+        byte[] serialized = dg.getBytes();
+        try (IonReader reader = IonReaderBuilder.standard().build(serialized)) {
+            assertEquals(IonType.STRUCT, reader.next());
+            reader.stepIn();
+            assertEquals(IonType.SYMBOL, reader.next());
+            assertEquals("foo", reader.getFieldName());
+            String[] annotations = reader.getTypeAnnotations();
+            assertEquals(2, annotations.length);
+            assertEquals("bar", annotations[0]);
+            assertEquals("abc", annotations[1]);
+        }
+    }
 }
diff --git a/test/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java b/test/com/amazon/ion/impl/IonReaderContinuableTopLevelBinaryTest.java
@@ -23,6 +23,7 @@
 import com.amazon.ion.SystemSymbols;
 import com.amazon.ion.TestUtils;
 import com.amazon.ion.Timestamp;
+import com.amazon.ion.UnknownSymbolException;
 import com.amazon.ion.impl.bin._Private_IonManagedBinaryWriterBuilder;
 import com.amazon.ion.impl.bin._Private_IonManagedWriter;
 import com.amazon.ion.impl.bin._Private_IonRawWriter;
@@ -769,6 +770,36 @@ public void invalidVersionMarker(boolean constructFromBytes) throws Exception {
         });
     }
 
+    @ParameterizedTest(name = "constructFromBytes={0}")
+    @ValueSource(booleans = {true, false})
+    public void unknownSymbolInFieldName(boolean constructFromBytes) throws Exception {
+        reader = readerFor(constructFromBytes, 0xD3, 0x8A, 0x21, 0x01);
+        assertSequence(next(IonType.STRUCT), STEP_IN, next(IonType.INT));
+        assertThrows(UnknownSymbolException.class, reader::getFieldNameSymbol);
+        assertThrows(UnknownSymbolException.class, reader::getFieldName);
+        reader.close();
+    }
+
+    @ParameterizedTest(name = "constructFromBytes={0}")
+    @ValueSource(booleans = {true, false})
+    public void unknownSymbolInAnnotation(boolean constructFromBytes) throws Exception {
+        reader = readerFor(constructFromBytes, 0xE4, 0x81, 0x8A, 0x21, 0x01);
+        assertSequence(next(IonType.INT));
+        assertThrows(UnknownSymbolException.class, reader::getTypeAnnotationSymbols);
+        assertThrows(UnknownSymbolException.class, reader::getTypeAnnotations);
+        reader.close();
+    }
+
+    @ParameterizedTest(name = "constructFromBytes={0}")
+    @ValueSource(booleans = {true, false})
+    public void unknownSymbolInValue(boolean constructFromBytes) throws Exception {
+        reader = readerFor(constructFromBytes, 0x71, 0x0A);
+        assertSequence(next(IonType.SYMBOL));
+        assertThrows(UnknownSymbolException.class, reader::symbolValue);
+        assertThrows(UnknownSymbolException.class, reader::stringValue);
+        reader.close();
+    }
+
     /**
      * Feeds all bytes from the given array into the pipe one-by-one, asserting before each byte that the reader
      * is not positioned on a value.

diff --git a/test/com/amazon/ion/system/IonBinaryWriterBuilderTest.java b/test/com/amazon/ion/system/IonBinaryWriterBuilderTest.java
@@ -24,16 +24,21 @@
 import static org.junit.Assert.fail;
 
 import com.amazon.ion.IonCatalog;
+import com.amazon.ion.IonReader;
 import com.amazon.ion.IonSystem;
+import com.amazon.ion.IonType;
 import com.amazon.ion.IonWriter;
 import com.amazon.ion.SymbolTable;
+import com.amazon.ion.TestUtils;
 import com.amazon.ion.impl.Symtabs;
 import com.amazon.ion.impl._Private_IonBinaryWriterBuilder;
 import com.amazon.ion.impl._Private_IonWriter;
 import com.amazon.ion.impl._Private_Utils;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.OutputStream;
+import java.nio.charset.StandardCharsets;
+
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -299,6 +304,40 @@ public void testImmutableInitialSymtab()
         assertTrue(symbolTableEquals(lst, writer.getSymbolTable()));
     }
 
+    @Test
+    public void testInitialSymtabFromReader()
+        throws Exception
+    {
+        SymbolTable lst;
+        try (IonReader reader = IonReaderBuilder.standard().build(TestUtils.ensureBinary(IonSystemBuilder.standard().build(), "foo".getBytes(StandardCharsets.UTF_8)))) {
+            assertEquals(IonType.SYMBOL, reader.next());
+            lst = reader.getSymbolTable();
+        }
+
+        _Private_IonBinaryWriterBuilder b =
+            _Private_IonBinaryWriterBuilder.standard();
+        b.setInitialSymbolTable(lst);
+        assertSame(lst, b.getInitialSymbolTable());
+
+        ByteArrayOutputStream out = new ByteArrayOutputStream();
+        IonWriter writer = b.build(out);
+        assertTrue(symbolTableEquals(lst, writer.getSymbolTable()));
+
+        writer = b.build(out);
+        assertTrue(symbolTableEquals(lst, writer.getSymbolTable()));
+
+        writer.writeSymbol("bar");
+        writer.writeSymbol("foo");
+        writer.close();
+
+        try (IonReader reader = IonReaderBuilder.standard().build(out.toByteArray())) {
+            assertEquals(IonType.SYMBOL, reader.next());
+            assertEquals("bar", reader.stringValue());
+            assertEquals(IonType.SYMBOL, reader.next());
+            assertEquals("foo", reader.stringValue());
+        }
+    }
+
     @Test(expected = UnsupportedOperationException.class)
     public void testInitialSymtabImmutability()
     {