Skip to content
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

Fixes an issue where offset capture of a multi-field struct was incor… #251

Merged
merged 3 commits into from
Jun 16, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ionc/ion_reader.c
Original file line number Diff line number Diff line change
Expand Up @@ -1986,6 +1986,11 @@ iERR _ion_reader_get_position_helper(ION_READER *preader, int64_t *p_bytes, int3
* ion version marker and the initial local symbol table (if one
* is present). At that point the symbol table will be current
* and later seek's will have an appropriate symbol table to use.
*
* Note that when seeking directly into a struct, the offsets
* provided by calls to ion_reader_get_value_offset point to the
* beginning of the value, not the field, and therefore the field
* name will not be accessible via ion_reader_get_field_name.
*/
iERR ion_reader_seek(hREADER hreader, POSITION offset, SIZE length)
{
Expand Down
4 changes: 4 additions & 0 deletions ionc/ion_reader_text.c
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,10 @@ iERR _ion_reader_text_next(ION_READER *preader, ION_TYPE *p_value_type)
// The start position of the value was not known at the start of this function. At this point it must be known.
text->_value_start = text->_scanner._value_start;
}
else if (text->_state == IPS_BEFORE_FIELDNAME) {
// In latter fields in a struct, value_start will be positive, but positioned at the field name, not the value
text->_value_start = text->_scanner._value_start;
}
else {
text->_value_start = value_start;
}
Expand Down
77 changes: 77 additions & 0 deletions test/test_ion_reader_seek.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -679,4 +679,81 @@ TEST_P(TextAndBinary, ReaderHandlesInitialUnannotatedContainerValueOffsetSeek) {
ION_ASSERT_OK(ion_reader_seek(reader, pos_second, -1));
ION_ASSERT_OK(ion_reader_next(reader, &type));
ASSERT_EQ(tid_SYMBOL, type);
}

TEST_P(TextAndBinary, ReaderPopulatesStructFieldsOnSeek) {
// Write!

hWRITER writer = NULL;
ION_STREAM *ion_stream = NULL;
ION_STRING field1, field2, value;
BYTE *data;
SIZE data_length;

// {field1:val1,field2:val2}
ion_string_from_cstr("field1", &field1);
ion_string_from_cstr("value", &value);
ion_string_from_cstr("field2", &field2);
ION_ASSERT_OK(ion_test_new_writer(&writer, &ion_stream, is_binary));
ION_ASSERT_OK(ion_writer_start_container(writer, tid_STRUCT));
ION_ASSERT_OK(ion_writer_write_field_name(writer, &field1));
ION_ASSERT_OK(ion_writer_write_symbol(writer, &value));
ION_ASSERT_OK(ion_writer_write_field_name(writer, &field2));
ION_ASSERT_OK(ion_writer_write_symbol(writer, &value));
ION_ASSERT_OK(ion_writer_finish_container(writer));
ION_ASSERT_OK(ion_test_writer_get_bytes(writer, ion_stream, &data, &data_length));

// Read!

hREADER reader = NULL;
ION_TYPE type;
POSITION pos_field1, pos_field2;
ION_STRING read_field1, read_val1, read_field2, read_val2;

// We use this reader to capture offsets
ION_ASSERT_OK(ion_test_new_reader(data, data_length, &reader));

// Assemble: Take one pass through the document to capture value offsets and field names

ION_ASSERT_OK(ion_reader_next(reader, &type));

ION_ASSERT_OK(ion_reader_step_in(reader));

ION_ASSERT_OK(ion_reader_next(reader, &type));
ASSERT_EQ(tid_SYMBOL, type);
ION_ASSERT_OK(ion_reader_get_value_offset(reader, &pos_field1));

ION_ASSERT_OK(ion_reader_next(reader, &type));
ASSERT_EQ(tid_SYMBOL, type);
ION_ASSERT_OK(ion_reader_get_value_offset(reader, &pos_field2));

ION_ASSERT_OK(ion_reader_step_out(reader));

// Act: Move back to the original offsets and then re-assemble values

// Seek to first field
ION_ASSERT_OK(ion_reader_seek(reader, pos_field1, -1));
ION_ASSERT_OK(ion_reader_next(reader, &type));
ASSERT_EQ(tid_SYMBOL, type);

// Read field value
ION_ASSERT_OK(ion_reader_read_string(reader, &read_val1));
char *cread_val1 = ion_string_strdup(&read_val1);

// Seek to second field
ION_ASSERT_OK(ion_reader_seek(reader, pos_field2, -1));
ION_ASSERT_OK(ion_reader_next(reader, &type));
ASSERT_EQ(tid_SYMBOL, type);

// Read field value
ION_ASSERT_OK(ion_reader_read_string(reader, &read_val2));
char *cread_val2 = ion_string_strdup(&read_val2);

ION_ASSERT_OK(ion_reader_close(reader));

// Assert:

// Easy assertions: there's only one value, "value," and we should have read it both times
assertStringsEqual((char *)value.value, cread_val1, strlen(cread_val1));
assertStringsEqual((char *)value.value, cread_val2, strlen(cread_val2));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably good enough, but I would feel better about it if the values weren't the same. That way we know this won't happen to pass if the second seek somehow leaves the reader positioned at the first value.

}