Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

Leading zeros lost from version when listing blank forms #327

Closed
cooperka opened this issue Oct 15, 2018 · 5 comments
Closed

Leading zeros lost from version when listing blank forms #327

cooperka opened this issue Oct 15, 2018 · 5 comments

Comments

@cooperka
Copy link

Re-filing getodk/collect#2296 under this repo.

Software and hardware versions

Aggregate v1.6.1

Problem description

Leading zeros are lost from version numbers when listing blank forms from the Aggregate API. For example, version 0001 is transmitted as simply 1.

Steps to reproduce the problem

  1. Visit the sandbox
  2. Upload example multi-warning.xml
  3. Retrieve the list of blank forms (e.g. via the Collect app)

The response sent back from the server for the Multi Warning form is:

<xform>
  <formID>multi warning</formID>
  <name>Multi Warning</name>
  <majorMinorVersion>1</majorMinorVersion>
  <version>1</version>
  <hash>md5:c1983e6a874af5cb49b4e484bfadb92d</hash>
  <downloadUrl>https://sandbox.aggregate.opendatakit.org/formXml?formId=multi+warning</downloadUrl>
</xform>

Whereas e.g. <version>500001</version> is correct when the leading character is non-zero.

Expected behavior

Leading zeros should not be lost.

@ggalmazor
Copy link
Contributor

Thanks for reporting the issue, @cooperka!

I know that Aggregate converts the version to a number to decide if an incoming form is newer than an already existing one.

What you report looks like Aggregate is storing the numeric representation instead of storing the literal version number coming on the blank form.

There are some things that come to mind:

  • Can we store the literal version number on the database?
  • Or do we need to add a field to the database to store both? << This could be problematic.

@cooperka
Copy link
Author

Thanks @ggalmazor. In my opinion, not having delved much into the documentation, it seems like we should either treat the version as a number or a string, and only store it once.

A. If it's a number, and we disallow any letters, stripping the zeros seems fair, but it should be documented that the version is simply a number and nothing more
B. If it's a string, and we allow letters, we should never convert, and instead sort alphanumerically (this algorithm, for example)

Requiring the version to be numeric but also allowing leading zeros seems like a complexity not worth supporting to me.

@ggalmazor
Copy link
Contributor

ggalmazor commented Oct 19, 2018

From what I gather looking to Aggregate's code, in XFormParameters the version string is parsed into a Long and then it's used to compare different instances of XFormParameters (to know if an instance is greater, equal, or less than another instance).

This is what the specs say about the orx:version attribute:

on the childnode of the primary instance in the http://openrosa.org/xforms/ namespace: Form version which can contain any string value. Like meta nodes this information is used as a processing cue for the server receiving the submission.

It looks like Aggregate is not conforming to spec...

@lognaturel
Copy link
Member

It looks like Aggregate is not conforming to spec...

I agree with that assessment and with @cooperka's proposed approaches. That said, I'm not sure whether it's high priority since it's such an unusual case and has not been reported by a user.

@ggalmazor
Copy link
Contributor

ggalmazor commented Oct 25, 2018

After studying this, and discussing it with @yanokwa and @lognaturel, it looks like an intervention to change how Aggregate deals with versions would be too risky at this moment.

For the moment, we've reviewed all the documentation available in search for spots where language should be changed and/or information should be added.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants