XML Version as File Modified time #431

aodhan-domhnaill · 2021-09-17T09:26:32Z

Description

#395

Code review items

Readable: Concise, well named, follows the style guide, documented if necessary.
Documented: Configuration and user documentation on cht-docs
Tested: Unit and/or integration tests where appropriate
Backwards compatible: Works with existing data and configuration. Any breaking changes documented in the release notes.

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

aodhan-domhnaill · 2021-09-17T09:27:22Z

src/lib/upload-forms.js

@@ -39,6 +39,7 @@ module.exports = async (projectDir, subDirectory, options) => {
    }

    const xml = fs.read(xformPath);
+    const xmlVersion = fs.statSync(filePath).mtime;


I'm not sure Git will preserve this correctly.

Yeah it looks like it doesn't preserve it. Given this I think we should instead... when building the form in cht-conf, if the output is different than the previous version, store the current time somewhere in a source controlled file.

There are some challenges with this approach... for example

If you repo is out of date then it may create a spurious version but spurious versions aren't the end of the world as long as everyone uses less-than and greater-than comparisons.

If the system clock is out of date the version will jump around. This should be rare especially in a CICD setup.

If you are okay adding a Git library, then we could use the Git mod time,

$ git log -1 --pretty="format:%ci" test/data/lib/upload-forms/merge-properties/forms/example.xml 2020-04-14 21:54:42 +1200

jkuester

Implementation looks good, but maybe I am missing something with regards to the actual functionality. How does give us the ability to find the exact version of the xlsx file which generated the form (like Kenn mentioned in his initial description of the issue #395)? AFIK the GitHub hash search only works for commit hashes and not hashes of the actual file contents. I tried searching for the hash of the test form xml (hash:7e3bb121779a8e9f707b6e1db4c1b52aa6e875b5015b41b0a9115efa2d0de1d1) but it did not give me anything useful.

aodhan-domhnaill · 2021-11-09T08:02:10Z

@kennsippell Can you comment about the hash search? If we use Git Commit, it might be better because we could have a timestamp too.

For my usage, I just need to know if the file changed.

kennsippell · 2021-11-12T18:42:55Z

Is it important to search by hash? - Our old data science team had this problem pretty frequently - looking at data and wanting to map it back to the source. "This user answered potatoes but the question is supposed to be an integer. What was the exact question we asked them?" Many times the answer is simply "we don't know". I do believe there is quite a bit of value in mapping CHT data back to the source...

How to do it

If the hashing algorithm we use is the same as GitHub, this would let users GitHub hash search for the exact version of the form that caused the report.

commit hash vs content hash - that's just my mistake. I think this comment from #395 is false. Even git's "pickaxe" doesn't seem to support searching by content hash. I suspect what you're hashing here can still be used to perform the search if somebody really wants to - just not through a convenient UI that I know of. Sorry to mislead. What you're doing here seems like a good option still.

But note the approach of SHA + MIN(timestamp) query might yield some funk if a form reverts to an exact previous state (A > B > A). I don't have a better suggestion.

jkuester · 2021-11-15T15:40:37Z

@aodhan-domhnaill @kennsippell Would it make sense to move this hashing code that we have here into convert-forms (maybe as part of the fixXml function) and store the xmlVersion hash maybe as another attribute on the model/instance/form_name element? Then the hash would get saved to the actual xml file (and get committed/version-controlled). Still would not enable GitHub searching, but anyone with the repo checked out locally could just use git log -G<regex> to search the whole revision history of the repo looking for commits containing file changes with the hash text.

kennsippell · 2021-11-15T16:42:19Z

Makes sense, but I'm not sure we are 100% on committing xml with xlsx changes. Currently with the xml being generated by the CI system, it isn't necessary. If we take this path - it'd be nice for the CI system to also forced the xml commit (either by committing it if different, or failing).

garethbowen · 2021-11-15T17:54:59Z

@aodhan-domhnaill I have concerns about using the hash alone because I think it'll add significant complexity in the pipeline configuration because the hash won't be sequential. If a form is modified 100 times and a field was added in version 50, you would have to check the hash against all 50 later hashes to work out if the field should be there or not. Furthermore if version 101 was added it'd be easy to forget to update the if-else conditions and the configuration would regress.

This would be much easier with a timestamp so the xml version can be compared with < and >. It sounds like from @kennsippell that the content hash isn't very useful because you can't do a search for the source, but if you'd still like to retain it then I think the xml version should be date + hash so the versions are sequential still.

aodhan-domhnaill · 2021-11-22T07:45:20Z

@garethbowen I would like a timestamp as well, but we can't use the Unix timestamp of the file. Using the timestamp off someones computer seems scary because timezones and local issues could cause a problem. Using the last Git commit for the file would be nice because that comes with a searchable hash and a timestamp. Alternatively, trusted timestamping is an option, but seems excessive.

@kennsippell Will this tool always be run in a Git repo? Is using a Git commit hash/timestamp feasible?

garethbowen · 2021-11-23T00:50:00Z

Timezones shouldn't be a problem as long as the timestamp is UTC. Local issues (such as computer clock out of sync) could be an issue but that shouldn't be common. I agree that trusted timestamps are overkill. We can't rely on the project using git or any change control at all, but we could try that and fall back to the local timestamp if necessary?

$ sha256sum test/data/lib/upload-forms/merge-properties/forms/example.xml 7e3bb121779a8e9f707b6e1db4c1b52aa6e875b5015b41b0a9115efa2d0de1d1 test/data/lib/upload-forms/merge-properties/forms/example.xml

jkuester · 2022-01-31T22:11:02Z

test/lib/upload-forms.spec.js

@@ -52,6 +53,8 @@ describe('upload-forms', () => {
      const form = await api.db.get('form:example');
      expect(form.type).to.equal('form');
      expect(form.internalId).to.equal('different');
+      expect(form.xmlVersion.time).to.equal(123123);
+      expect(form.xmlVersion.sha256).to.equal('7e3bb121779a8e9f707b6e1db4c1b52aa6e875b5015b41b0a9115efa2d0de1d1');


Probably worth leaving a comment in here just to call out that if this assert fails (and you just made changes to the example.xml), then this is expected behavior and you can just update the expected has value....

jkuester

🚀

aodhan-domhnaill commented Sep 17, 2021

View reviewed changes

This was referenced Sep 17, 2021

Update convert-form action to include a hash which maps report back to the source form #395

Closed

Adding XML Version medic/cht-core#7319

Merged

aodhan-domhnaill marked this pull request as ready for review November 8, 2021 12:52

aodhan-domhnaill force-pushed the 395-xml-form-version branch from e7a6711 to df2fe7a Compare November 8, 2021 12:54

aodhan-domhnaill requested review from jkuester and kennsippell November 8, 2021 12:56

jkuester reviewed Nov 8, 2021

View reviewed changes

Aidan Macdonald and others added 4 commits December 31, 2021 12:25

XML Version as File Modified time

3b50c7f

Add hash

d7eb6ab

$ sha256sum test/data/lib/upload-forms/merge-properties/forms/example.xml 7e3bb121779a8e9f707b6e1db4c1b52aa6e875b5015b41b0a9115efa2d0de1d1 test/data/lib/upload-forms/merge-properties/forms/example.xml

Only putting hash in xmlVersion

9659176

Fixing time stamp

47cd304

aodhan-domhnaill force-pushed the 395-xml-form-version branch from 0b16234 to 47cd304 Compare December 31, 2021 10:26

jkuester reviewed Jan 31, 2022

View reviewed changes

jkuester approved these changes Jan 31, 2022

View reviewed changes

aodhan-domhnaill merged commit 9d0b8d7 into master Feb 2, 2022

aodhan-domhnaill deleted the 395-xml-form-version branch February 2, 2022 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

XML Version as File Modified time #431

XML Version as File Modified time #431

aodhan-domhnaill commented Sep 17, 2021 •

edited

Loading

aodhan-domhnaill Sep 17, 2021

garethbowen Sep 19, 2021

aodhan-domhnaill Sep 21, 2021

jkuester left a comment •

edited

Loading

aodhan-domhnaill commented Nov 9, 2021

kennsippell commented Nov 12, 2021 •

edited

Loading

jkuester commented Nov 15, 2021

kennsippell commented Nov 15, 2021 •

edited

Loading

garethbowen commented Nov 15, 2021

aodhan-domhnaill commented Nov 22, 2021

garethbowen commented Nov 23, 2021

jkuester Jan 31, 2022

jkuester left a comment

XML Version as File Modified time #431

XML Version as File Modified time #431

Conversation

aodhan-domhnaill commented Sep 17, 2021 • edited Loading

Description

Code review items

License

aodhan-domhnaill Sep 17, 2021

Choose a reason for hiding this comment

garethbowen Sep 19, 2021

Choose a reason for hiding this comment

aodhan-domhnaill Sep 21, 2021

Choose a reason for hiding this comment

jkuester left a comment • edited Loading

Choose a reason for hiding this comment

aodhan-domhnaill commented Nov 9, 2021

kennsippell commented Nov 12, 2021 • edited Loading

jkuester commented Nov 15, 2021

kennsippell commented Nov 15, 2021 • edited Loading

garethbowen commented Nov 15, 2021

aodhan-domhnaill commented Nov 22, 2021

garethbowen commented Nov 23, 2021

jkuester Jan 31, 2022

Choose a reason for hiding this comment

jkuester left a comment

Choose a reason for hiding this comment

aodhan-domhnaill commented Sep 17, 2021 •

edited

Loading

jkuester left a comment •

edited

Loading

kennsippell commented Nov 12, 2021 •

edited

Loading

kennsippell commented Nov 15, 2021 •

edited

Loading