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

Validate json for importing and exporting TTL data #946

Closed
wants to merge 13 commits into from

Conversation

SarahRemus
Copy link
Contributor

Related issue

towards #932

Context / Background

We are currently developing the functionality of syncing the TTL data to Google Drive, related to #913.

When importing the data, it would be good to validate if the given JSON file is in the correct TTL format. As discussed in #932 this functionality can also be useful in other parts of the project.

What change is being introduced by this PR?

I implemented the functionality to check whether a JSON file is in the correct TTL format. For that I oriented my self on the code in import-export.js as some form of a validation is also done there, however it's more intertwined with the rest of the code and not easily reusable.
There are some functions in time-math.js I could have used, but I decided to reimplement them as they have the following flaws (in my opinion, maybe I'm overlooking something):

  • the isValidDayOfMonth(dayOfMonth, month) function can't handle leap years
  • I'm pretty sure the regex used in validateDate(date) is only going to November and not December
  • (1|2)[0-9]{3}-(0[1-9]{1}|1 [0-1] {1})-(0[0-9]{1}|1[0-9]{1}|2[0-9]{1}|3[0-1]{1})

How will this be tested?

I wrote a bunch of unit tests, testing all kinds of different data input. But we definitely have to do more tests once the functionality is integrated with the sync-data-to-google-drive functionality.

Copy link
Owner

@thamara thamara left a comment

Choose a reason for hiding this comment

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

Hi @SarahRemus, thanks for your patience with this (and others) PR.

I really liked the idea of the json scheme validator, but was under the impression we needed to sort out the whole google drive thing first.
But looking at it now, I believe this PR can already be included, and we should seek a further work to replace our hand-made json validator when importing a database with this cleaner solution.

Please let me know if you'd like to still work on this PR, otherwise, I can include the suggest changes and merge it.

js/validate-json.js Outdated Show resolved Hide resolved
__tests__/__main__/validate-json.js Show resolved Hide resolved
js/validate-json.js Outdated Show resolved Hide resolved
__tests__/__main__/validate-json.js Show resolved Hide resolved
js/validate-json.js Outdated Show resolved Hide resolved
js/validate-json.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@thamara
Copy link
Owner

thamara commented Jan 8, 2024

Actually, the changes were very minor, so I was able to quickly address them and will merge this shortly.

js/validate-json.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
__tests__/__main__/validate-json.js Outdated Show resolved Hide resolved
thamara and others added 3 commits January 7, 2024 23:34
Co-authored-by: Arthur Araujo <37311270+araujoarthur0@users.noreply.github.com>
Co-authored-by: Arthur Araujo <37311270+araujoarthur0@users.noreply.github.com>
Co-authored-by: Arthur Araujo <37311270+araujoarthur0@users.noreply.github.com>
@thamara
Copy link
Owner

thamara commented Jan 8, 2024

\changelog-update
Message: Enhancement [#946]: Included formal json scheme validator
User: SarahRemus

@thamara
Copy link
Owner

thamara commented Jan 8, 2024

This was merged, but because it was merged directly into main, GH is not able to identify it. So closing it.
Thanks @SarahRemus!

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