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

Add partial elements #775

Merged
merged 1 commit into from
Jul 10, 2024
Merged

Add partial elements #775

merged 1 commit into from
Jul 10, 2024

Conversation

seadowg
Copy link
Member

@seadowg seadowg commented Jul 8, 2024

Work towards getodk/collect#5623

This adds the ability to provide "partial" parses of external instances using a custom FileInstanceParser:

class MyFileInstanceParser implements ExternalInstanceParser.FileInstanceParser {

        @Override
        public TreeElement parse(@NotNull String instanceId, @NotNull String path) throws IOException {
            return parse(instanceId, path, false);
        }

        @Override
        public TreeElement parse(@NotNull String instanceId, @NotNull String path, boolean partial) throws IOException {
            TreeElement root = new TreeElement("root", 0);

            TreeElement value = new TreeElement("value");
            TreeElement label = new TreeElement("label");

            if (!partial) {
                value.setValue(new StringData("blah"));
                label.setValue(new StringData("Blah"));
            }

            TreeElement item = new TreeElement("item", 0, partial);
            item.addChild(value);
            item.addChild(label);

            root.addChild(item);
        }
}

JavaRosa will allow partial parses for parsing/deserializing forms (by passing partial as true), but will require a "full" parse (partial as false) if it needs to resolve a reference currently involving a partial TreeElement.

Clients can also populate partial elements at an instance level by calling DataInstance#populatePartialElements. This will allow plugins to populate partials before they are resolved to avoid full parses (from a FilterStrategy for example).

What has been done to verify that this works as intended?

New tests. I've also built a toy implementation in Collect to validate that the API feels correct.

Why is this the best possible solution? Were any other approaches considered?

There's a few different approaches that I've discussed in different chats and also in getodk/collect#5623 (like fallbacks for individual elements rather than full parses for example). This approach has ended up winning out, so unless there's anything that feels incorrect here I don't think there's that much that needs to be pointed out.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

There should be very little risk here as the new code will only kick in if "activated" by a client. The big risk is that there's some edge case I've missed that we'll run into down the road when adding in these new features.

@@ -26,6 +26,5 @@ Inspect external instances (their ID and parsed XML) after parsing or provide cu

### API
- `ExternalInstanceParser#addFileInstanceParser`
- `ExternalInstanceparsser#addProcessor`
Copy link
Member Author

Choose a reason for hiding this comment

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

It feels like addFileInstanceParser replaced this, so I wanted to take the opportunity to clean up the API.

@seadowg seadowg marked this pull request as ready for review July 9, 2024 17:13
@seadowg seadowg requested a review from lognaturel July 9, 2024 17:13
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

I still don’t feel fully comfortable with having to first create partial elements and then fill them in as opposed to only creating the elements that need to exist. But let’s experiment with this and see how it feels and performs, I may very well warm up to it! I’ll also refresh my memory on the reference verification that happens at form parse time.

Slight preference for squashing.

new Pair<>("1", "Item 1")
), true));

File tempFile = TempFileUtils.createTempFile("fale-instance", "fake");
Copy link
Member

Choose a reason for hiding this comment

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

Fale -> fake

@seadowg
Copy link
Member Author

seadowg commented Jul 10, 2024

I still don’t feel fully comfortable with having to first create partial elements and then fill them in as opposed to only creating the elements that need to exist. But let’s experiment with this and see how it feels and performs, I may very well warm up to it! I’ll also refresh my memory on the reference verification that happens at form parse time.

Agreed! I realized when working on 5f9fc09 that our current implementation should also work for a version where a FileInstanceParser produces totally empty item elements. It's just the verification that's blocking us. Hopefully we can remove/move that so we can get that extract but of gains.

@seadowg seadowg merged commit 882d0f8 into getodk:master Jul 10, 2024
3 checks passed
@seadowg seadowg deleted the partials branch July 10, 2024 08:06
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.

2 participants