Skip to content

Allow changing blob attributes when full/custom validation is enabled#1064

Merged
stevedlawrence merged 1 commit into
apache:mainfrom
stevedlawrence:daffodil-2837-full-validation-blobs
Aug 4, 2023
Merged

Allow changing blob attributes when full/custom validation is enabled#1064
stevedlawrence merged 1 commit into
apache:mainfrom
stevedlawrence:daffodil-2837-full-validation-blobs

Conversation

@stevedlawrence
Copy link
Copy Markdown
Member

When full or custom validation is enabled, Daffodil creates a new TeeInfosetOutputter which proxies infoset events to the user provided InfosetOutputter and another one used for validation. This means that calls to getBlobDirectory, getBlobSuffix, setBlobPath, etc. go to the TeeInfosetOutputter, and ignore any values that a user may have set on their provided InfosetOutputter. This essentially means you cannot change the directory, suffix, or prefix of blobs if full or custom validation is enabled. It also means that if calling getBlobPaths on the user provided InfosetOutputter never returns anything, even if blobs were created.

To fix this, if full or custom validation is enabled, we copy the blob attributes to the TeeInfosetOutputter used for parsing. We also ensure that at the end of parsing, we call setBlobPaths on the InfosetOutputter the user passed in instead of the TeeInfosetOutputter.

DAFFODIL-2837

@stevedlawrence stevedlawrence force-pushed the daffodil-2837-full-validation-blobs branch 2 times, most recently from 8d2249b to b0706bf Compare August 3, 2023 20:16
Copy link
Copy Markdown
Contributor

@tuxji tuxji left a comment

Choose a reason for hiding this comment

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

+1, looks good to me

Copy link
Copy Markdown
Contributor

@mbeckerle mbeckerle left a comment

Choose a reason for hiding this comment

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

+1

assertTrue(blobPaths.get(0).toString().contains("pre-"));
assertTrue(blobPaths.get(0).toString().contains(".suf"));
Files.delete(blobPaths.get(0));
Files.delete(blobDir);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this test fails it will leave files/dirs around? Do you want a try-finally here to ensure they are cleaned up?

It doesn't much matter since the test should not fail.

It's just a matter of "what if someone copies this code to model their blob usage?". Then it would be better if the code had the try-finally.

(Same comment for the Scala API test file.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Makes sense, will add.

When full or custom validation is enabled, Daffodil creates a new
TeeInfosetOutputter which proxies infoset events to the user provided
InfosetOutputter and another one used for validation. This means that
calls to getBlobDirectory, getBlobSuffix, setBlobPath, etc. go to the
TeeInfosetOutputter, and ignore any values that a user may have set on
their provided InfosetOutputter. This essentially means you cannot
change the directory, suffix, or prefix of blobs if full or custom
validation is enabled. It also means that if calling getBlobPaths on the
user provided InfosetOutputter never returns anything, even if blobs
were created.

To fix this, if full or custom validation is enabled, we copy the blob
attributes to the TeeInfosetOutputter used for parsing. We also ensure
that at the end of parsing, we call setBlobPaths on the InfosetOutputter
the user passed in instead of the TeeInfosetOutputter.

DAFFODIL-2837
@stevedlawrence stevedlawrence force-pushed the daffodil-2837-full-validation-blobs branch from b0706bf to 28b1a0d Compare August 4, 2023 15:09
@stevedlawrence stevedlawrence merged commit 3ae1ef4 into apache:main Aug 4, 2023
@stevedlawrence stevedlawrence deleted the daffodil-2837-full-validation-blobs branch August 4, 2023 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants