Issue 390 external instances #394
Conversation
…atches that of an internal secondary instance.
…sponding to the test class’s package name (as is our convention). No coding changes are needed because the method we use to locate the files, `r`, looks in the package name directory and then in `resources`.
…instance files. Add overloaded `getFormFromInputStream(InputStream is, String externalInstancePathPrefix)` to XFormUtils. Modify test code, FormParserHelper, to use XFormUtils.getFormFromInputStream so we are testing the same path that Collect uses. Added overloaded `ResourcePathHelper.r(String filename, boolean fallBack)` to allow building names of files that don’t exist (such as those that are generated).
Codecov Report
@@ Coverage Diff @@
## master #394 +/- ##
============================================
+ Coverage 48.31% 48.45% +0.14%
- Complexity 2871 2883 +12
============================================
Files 239 239
Lines 13516 13555 +39
Branches 2625 2615 -10
============================================
+ Hits 6530 6568 +38
- Misses 6148 6149 +1
Partials 838 838
Continue to review full report at Codecov.
|
…nstance as an example
There was a problem hiding this comment.
Thanks, @dcbriccetti! I think that, overall, this is a really good approach that solves the issue. I'm comfortable with the required change in the API too.
I don't know whether you just wanted to confirm this or you wanted a full head-on review, so I did that as well. I made some comments about some changes I think we need to discuss. Let me know what you thnk! :)
| * class’s corresponding directory | ||
| * @return a Path for the resource file | ||
| */ | ||
| public static Path r(String filename, boolean fallBack) { |
There was a problem hiding this comment.
I think we should throw an error when a test asks for a file that isn't there. In fact, this implementation in masking an error on this PR: the file towns-large.xmlis not present (I'm guessing you forgot to commit the file?), but the timing test still passes.
I like the fallback mechanism though, and I think we should have it as the default behavior, which would avoid some changes on the test classes. I've tried this implementation, which works:
public static Path r(String filename) {
String resourceFileParentPath = inferResourceFileParentPath();
Path resourceFilePath = Paths.get("resources", resourceFileParentPath, filename);
if (Files.exists(resourceFilePath))
return resourceFilePath;
Path resources = Paths.get("resources", filename);
if (Files.exists(resources))
return resources;
throw new RuntimeException("File " + filename + " not found");
}There was a problem hiding this comment.
Now I think I'd like to change the implementation to recursively search for the file with Files.walk, but I'm not super crazy about it either.
There was a problem hiding this comment.
towns-large.xml is dynamically generated, hence it might not exist when r is called.
There was a problem hiding this comment.
Ugh, I didn't realize that... sorry!
In any case, what do you think about making it the default behavior and avoid having two parse methods?
This implementation works on all the test scenarios without polluting unknown paths:
public static Path r(String filename) {
String resourceFileParentPath = inferResourceFileParentPath();
Path resourceFilePath = Paths.get("resources", resourceFileParentPath, filename);
if (Files.exists(resourceFilePath))
return resourceFilePath;
Path resources = Paths.get("resources", filename);
if (Files.exists(resources))
return resources;
// File doesn't exist. Returning a temp file
String name = filename.substring(0, filename.lastIndexOf("."));
String extension = filename.substring(filename.lastIndexOf("."));
try {
return Files.createTempFile(name + "-", extension);
} catch (IOException e) {
throw new UncheckedIOException(e);
}
}This code would create a /tmp/towns-large-8296528288103120809.xml, for example.
|
I have @cooperka’s forms (see the issue) in Collect, and I’m trying to get them to work. We fail to generate the dynamic options for the second page after pushing a radio button on the first page and swiping to advance. The predicates aren’t matching in |
|
OK, that problem is solved. We weren’t setting the instance ID in the ExternalDataInstance’s root nodes. The next problem to solve may be opening a saved form with external data. Here’s a bit of the stack trace I’ll be examining: |
…nce. Now forms can be saved and re-edited.
|
OK, I’ve solved that last problem. We weren’t (de)serializing the external instance’s data. |
|
I just realized we probably don’t want to serialize the contents of the external instance. Rather, we should re-parse it upon deserialization of the form it’s in. |
- The XFormUtils.getFormFromInputStream() method won't let us listen to parse errors/warnings and we need to be able to check them
- Now the RootTranslator won't derive URI that already starts with the translated URI - Added ResourceManager unit tests
…renceManager to find external instance files.
…form is cached. Log a message and throw an XFormParseException when external instance parsing fails.
|
That’s the way I first wrote it. But: |
| return root; | ||
| } | ||
| } | ||
| for (List<? extends ReferenceFactory> rfs : Arrays.asList(sessionTranslators, translators, factories)) |
There was a problem hiding this comment.
You could do this:
private ReferenceFactory derivingRoot(String uri) throws InvalidReferenceException {
for (ReferenceFactory rf : getAllReferenceFactoriesInOrder())
if (rf.derives(uri))
return rf;
throw new InvalidReferenceException(getPrettyPrintException(uri), uri);
}
private List<ReferenceFactory> getAllReferenceFactoriesInOrder() {
List<ReferenceFactory> refFactories = new ArrayList<>();
refFactories.addAll(sessionTranslators);
refFactories.addAll(translators);
refFactories.addAll(factories);
return refFactories;
}What do you think?
We could bump the List subtype to a LinkedList to actually enforce order.
There was a problem hiding this comment.
List<? extends ReferenceFactory must really bother you, if you would replace it with that much code. It doesn’t bother me a bit. It’s a list of types that extend ReferenceFactory. I’d rather keep it the way I have it if you don’t feel strongly about it.
There was a problem hiding this comment.
No problem!
I can't help to not try to fix design problems. Those subtypes are leaking because that hierarchy is not sealed tight. The code I'm suggesting is more verbose but deals with that design flaw. It also flattens the lists, which is kind of nice too.
I won't cry if you don't change it :)
There was a problem hiding this comment.
Thank you for not crying.
Sometime I’d like to understand the design flaw this would deal with.
There was a problem hiding this comment.
(I don't know why a dagger was showing up in my last comment!)
I agree that my suggestion doesn't really solve the core problem. It's only an aesthetic change. Dealing with the core problem would require deeper work.
This is something super common not only in the ODK codebase, but in many other projects as well, and I think we can blame Java and how Java has been historically teached for that.
The flaw is that the hierarchy formed by ReferenceFactory (supertype) and its subtypes RootTranslator, PrefixedRootFactory, ResourceReferenceFactory, ..., is leaking the subtypes.
You can see this leak in many places. Related to this change we're talking about:
- You can't change the type of the
ReferenceManagertranslatorsandsessionTranslatorsto aList<ReferenceFactory>because it's coupled with a method onRootTranslator. - Because of this, you can't directly get a
List<List<ReferenceFactory>>as a result of anArray.asList(sessionTranslators, translators, factories).
If you're a SOLID design fan, this is breaking the L (Liskov's Substitution Principle), but in a more general way, this is wrong because it's a misuse of inheritance.
Inheritance is a tool we use to implement polymorphism, and here we don't have polymorphism, which is weird because we could benefit from it (the for block we're discussing would benefit from it).
In a polymorphic type, you can't have the subtypes leaked outside the type hierarchy (no one can be coupled to them, even implicitly, like with the List<? extends ReferenceFactory>).
If this is not a polymorphic type, this is telling me that:
- Maybe we have used inheritance for code reuse, which would be much simpler to get by composing stuff together
- Maybe we have used inheritance systematically without taking into account the impact this has in evolving software and the cost of introducing new changes.
This is even worse because the ReferenceFactory is an extension point that clients use to adapt JR to their particular environment. It's really confusing that JR is coupled both to the supertype (OK) and the subtypes (wrong).
Since this is all across the codebase, it's not super critical to fix in this particular instance but you know the Boy Scout's rule: leave the place cleaner than you found it.
There was a problem hiding this comment.
This is very educational!
|
Even though there are two standing discussions (the one about I've also tested this in Collect, so I'm confident this is a step in the right direction. @dcbriccetti, if you feel the same, I'd suggest we merge so that it's easier for you to continue with #397 |
This includes IDEA-performed changes, a deprecation, and one hand-written optimization.
dcbriccetti
changed the title
Closes #390
What has been done to verify that this works as intended?
New tests using @cooperka’s forms are passing. Editing, saving, and re-editing Kevin’s forms in Collect, with this change, getodk/collect#2771, appears to be working.
Why is this the best possible solution? Were any other approaches considered?
I’ve filled in the missing pieces, now that I better understand how things are supposed to work.
Are there any risks to merging this code? If so, what are they?
This changes where JavaRosa looks for external secondary instance files, so any client relying on the previous behavior will need to be changed.