Updated Valijson to version 0.7, adding date validation for manifests.

[jedieaston]

• [x] Have you signed the Contributor License Agreement?
• [x] Are you working against an Issue?

---

Resolves #1725.

This PR updates Valijson to version 0.7, which includes validation for dates and times in JSON Schemas. Without any other changes to winget, this has resolved my core ask from that issue, which was making sure that ReleaseDates were actually valid dates.

Valijson was updated via git subtree pull -P src/Valijson/valijson https://github.com/tristanpenman/valijson v0.7 --squash, just like last time. There was a merge conflict (since we don't have the tests for Valijson in the subtree in this repo), so I just kept the deleted files deleted.

@denelon, unless you think we need a better error message I'm happy with this being the fix. I can add an error message in an upcoming PR if you want (so that there aren't other changes mixed in with the Valijson update). If so, I'll make sure the issue doesn't close.

Behold:

PS C:\Users\easton> cat .\manifest.yaml
# yaml-language-server: $schema=https://aka.ms/winget-manifest.singleton.1.1.0.schema.json
PackageIdentifier: Microsoft.VisualStudio.2022.BuildTools
PackageVersion: 17.0.1
PackageName: Visual Studio Build Tools 2022 Current
Publisher: Microsoft
License: Copyright (c) Microsoft Corporation. All rights reserved.
LicenseUrl: https://visualstudio.microsoft.com/license-terms/
Tags:
- C++
- tools
- C#
- build
- vs
Moniker: vs2022-buildtools
ReleaseDate: "Uh, last Tuesday I think?"
ShortDescription: These Build Tools allow you to build Visual Studio projects from a command-line interface. Use of this tool requires a valid Visual Studio license.
PackageUrl: https://visualstudio.microsoft.com/
Installers:
- Architecture: x64
  InstallerUrl: https://download.visualstudio.microsoft.com/download/pr/8cea3871-c742-43fb-bf8b-8da0699ab4af/25c54d66f5cf07b14cdc0d6dab2e3d5da7ec22dead4757e69011bb2b2946e384/vs_BuildTools.exe
  InstallerSha256: 25C54D66F5CF07B14CDC0D6DAB2E3D5DA7EC22DEAD4757E69011BB2B2946E384
  InstallerType: exe
  InstallerSwitches:
    Silent: --quiet
    SilentWithProgress: --passive
    Custom: --wait
    Upgrade: update
PackageLocale: en-US
ManifestType: singleton
ManifestVersion: 1.1.0
PS C:\Users\easton> wingetdev validate .\manifest.yaml
Manifest validation failed.
Manifest Error: Schema validation failed.
Error context: <root>[ReleaseDate] Description: String should be a valid date
Error context: <root> Description: Failed to validate against schema associated with property name 'ReleaseDate'.
 File: manifest.yaml

PS C:\Users\easton>

Microsoft Reviewers: Open in CodeFlow

[JohnMcPMS]

This doesn't have the actual subtree commits that I would expect, containing the subtree metadata that is needed for future pulls.

[jedieaston]

I squashed, which is what we did last time. Do you want me to include the entire history? If so I can force push onto this branch.

[JohnMcPMS]

You should --squash the subtree, but not the two commits the subtree command creates. Your last PR (#1721) has both of those commits.

[jedieaston]

Git doesn't seem to generate the two usual commits from a subtree pull --squash if there's been changes to the subtree in the repo you're pulling into, since there's a merge conflict. #2232 and #1871 both make changes to the subtree that Git can't merge since the files were also modified in the Valijson repo, and the only way I was able to resolve this was by reverting those two commits. I can reapply them onto this branch after the Valijson pull, but if you need to merge this branch into master without squashing it's going to clutter the history up.

If you have any tips on how I can fix this please let me know, this is getting farther than my Git knowledge usually goes :/ I've force pushed my most recent idea onto this branch.

[denelon]

@JohnMcPMS is my resident Git expert :)

[JohnMcPMS]

I was able to do it like this:

> git subtree pull -P src/Valijson/valijson https://github.com/tristanpenman/valijson v0.7 --squash
> git rm <each of the files as they are all removes>
> git commit

I can create a new PR or push to this if you want (or you can recreate it).

[jedieaston]

~~That isn't working for me, maybe something's up with my Git. After pulling and git rm-ing the conflicting files git commit is only showing a single merge commit for me:

If you would like to force push onto this branch you're welcome to (I have maintainer permissions turned on), or if it's easier to open your own PR then I can close this. (Unless you have any other quick ideas on what I'm doing wrong)~~

Edit: nope, I'm just dumb. it worked!

[jedieaston]

Nope, I'm just very very silly and can't git log. I think it looks fine now if you want to take another look.

[JohnMcPMS]

/azp run

[JohnMcPMS]

Looks good now.

[azure-pipelines[bot]]

Azure Pipelines successfully started running 1 pipeline(s).

[JohnMcPMS]

Looks like it is "working", but we have some invalid datetimes in our test data:

FAILED:
  REQUIRE( importOutput.str().find(Resource::LocString(Resource::String::ImportCommandReportDependencies).get()) != std::string::npos )
with expansion:
  4294967295 (0xffffffff)
  !=
  4294967295 (0xffffffff)
\x1B[91mJSON file is not valid
\x1B[91mSchema validation failed.
Error context: <root>[CreationDate] Description: String should be a valid date-time
Error context: <root> Description: Failed to validate against schema associated with property name 'CreationDate'.


at D:\a\1\s\src\AppInstallerCLITests\WorkFlow.cpp(2617)

Which brings up the next question, are we ok with a breaking change here? I don't know if it only breaks our test data, or if it would also break files in the wild. @denelon, I think this is now enforcing something that has been in the schema the whole time, but with no validation. Even so, it is a potential break in functionality, and I think we need to understand how much impact it would have before we check this in.

[denelon]

Do we have a method to test?

[JohnMcPMS]

I think it would be:

1. auditing every schema that we have
2. finding all fields that would now be validated
3. determining in which cases a bad value would now fail
4. determining in which cases we would have a bad value

For 4, I'm thinking of things like files generated by tools vs by hand. And if/when those tools may have changed whether they were generating bad or good values.

I'm less concerned about manifests being rejected by winget-pkgs and more about import files, but at the same time, if we do have manifest fields that would start failing, we could really be causing some pain when this hits the service.

[Trenly]

Thinking about this further - Is this something that can be applied only to specific manifest versions? E.g - Can 1.0.0 to 1.2.0 be validated as string, and 1.4.0 be validated as date, to prevent it from breaking existing manifests, but being enforced moving forward?

[jedieaston]

My only hesitancy with that is that this has been a rule in the schema from when ReleaseDates were first a thing. The breakage is really with winget validate, not with the manifest format. If winget validate is the canonical source of truth for whether something is a valid winget manifest or not, then we probably should have some different validation rules, but if we think the schema is the source of truth then this is just better enforcement of an existing rule.

[denelon]

@jedieaston is this waiting on us for review or something else?

[yao-msft]

@jedieaston is this waiting on us for review or something else?

This is breaking existing tests and also we don't know how many existing manifests have the date field violation. We need to go through the entire pkgs repo and fix violating manifests before this can be merged.

[Trenly]

This is breaking existing tests and also we don't know how many existing manifests have the date field violation. We need to go through the entire pkgs repo and fix violating manifests before this can be merged.

@yao-msft - Repology recently removed WinGet due to failing parsing of a date. Based on the commit message, and my own review of the manifests, it seems that all other manifests in winget-pkgs have valid dates. I did correct the invalid date mentioned in the commit, so all manifests at pkgs should now be correct. If @jedieaston fixes the existing tests, would this PR be able to move forward?

[yao-msft]

Since this pr is 2 years old, and valijson has 1.0.2 release already, I think we can start a new pr from scratch to update to valijson 1.0.2. The commits should be ideally as few as possible (probably 4 commits: squashed commits...; merge squashed commits; update readme|cgmanifest etc; fix testes). But before sending the pr, we'll need to make sure all manifests work with valijson 1.0.2. And we'll probably want to update the service with new changes quickly. I guess it requires some level of e2e effort.

[yao-msft]

Just had a discussion with the team, I'll work on this right away.

[JohnMcPMS]

Replaced by #4588