Ensure file names are valid before saving

[Trenly]

• Resolves Validate file name before downloading #84

@jedieaston @OfficialEsco @vedantmgoyal2009

I think this should work. I did some light testing on my own machine. If you could please review and test this quite carefully, this is one of the core parts of the script

URL Known to cause issues previously - https://pinyin.thunisoft.com/webapi/v1/setup/downloadSetup?setupid=21f0e88683894ebcb440759c5aa25c47&filename=HuayuPY-V7.2.0.213.exe

[jedieaston]

Typo.

BTW: Do you have a test link we can use that'll trigger the invalid extension logic?

[Trenly]

Typo.

BTW: Do you have a test link we can use that'll trigger the invalid extension logic?

I don't know that I have one that will; I'll try to find one though

[jedieaston]

Works well for me.

[Trenly]

Typo.

BTW: Do you have a test link we can use that'll trigger the invalid extension logic?

I was able to find one that caused an error - https://static.getaether.net/Releases/Aether-2.0.0-dev.15/2011262249.19338c93/win/Aether-Setup-2.0.0-dev.15%2B2011262249.19338c93.exe

Should be fixed with the latest push

[Trenly]

Uh oh; Looks like it doesn't like any sourceforge url's I'll have to look into that

[vedantmgoyal9]

Can't we simply rename as {PackageIdentifier}.{PackageVersion}.{extension}

Reference: microsoft#29454 (comment)

[Trenly]

Can't we simply rename as {PackageIdentifier}.{PackageVersion}.{extension}

Reference: microsoft#29454 (comment)

Technically, yes, but then that operates under the assumption that we can actually get the extension from the URL, which may not always be possible.

By downloading the binary without saving it, we can see if the web server has specified the file name. If the name is specified, we should use that name. If we didn’t use that name, all of the automatic architecture checking would never happen or it would need to be reworked. If the name isn’t specified, or the name the server provided isn’t valid, that is when we should fall back to the PackageIdentifier.PackageVersion.extension.

However, even with that fallback, we can’t guarantee that a file extension was provided. We can try getting it from the provided file name, but it might not exist there. So, we can try getting it from the URL; if that doesn’t work, then we have to use our own extension. We could technically not have an extension, but having a last case extension makes it so users can better understand what the files in their temp directory are if they choose to save them

[vedantmgoyal9]

Off-topic, is Moniker unknown?

cc @jedieaston

[Trenly]

Off-topic, is Moniker unknown?

No idea what causes that; It doesn’t seem to have any issues in the validation pipeline, and I can’t figure out any way to reliably replicate it

[jedieaston]

Moniker is only valid in files that are of ManifestType defaultLocale or singleton. The most likely reason for that error is that you have a file of type locale that has a Moniker in it. (In fact, the only difference between defaultLocale and locale is the Moniker field).

[Trenly]

Moniker is only valid in files that are of ManifestType defaultLocale or singleton. The most likely reason for that error is that you have a file of type locale that has a Moniker in it. (In fact, the only difference between defaultLocale and locale is the Moniker field).

Makes sense! I’ll update the logic to remove moniker when manifest type is locale

[vedantmgoyal9]

I guess you should lower the limit of the message. (8 to 128 characters would be fine, I think 🤔)

[Trenly]

I guess you should lower the limit of the message. (8 to 128 characters would be fine, I think 🤔)

@jedieaston what do you think? I personally think that things like 404 not found don't really provide much description as to what was not found, but I'd like to know what others think

[jedieaston]

240 characters (a tweet) should be enough? I think if you're going to make it as low as 128 characters, you should pop the link up to them to go and edit it (gh pr create prints it to stdout if you can snag it that way, or else calculate it?).

Explaining why is important, at least to me (I usually assume Levvie or Esco or y'all know what you're doing, but if someone that doesn't contribute that often wants to nuke an entire ID, they should at least explain in the PR if not open an issue about it).

[OfficialEsco]

Moniker is only valid in files that are of ManifestType defaultLocale or singleton. The most likely reason for that error is that you have a file of type locale that has a Moniker in it. (In fact, the only difference between defaultLocale and locale is the Moniker field).

I've actually brought this up earlier, the Locale Schema does actually not say that Moniker is a invalid variable

However winget validate and the bot have always showed a warning for it

I guess you should lower the limit of the message. (8 to 128 characters would be fine, I think 🤔)

@jedieaston what do you think? I personally think that things like 404 not found don't really provide much description as to what was not found, but I'd like to know what others think

I think 404 not found is a good enough description instead of having to type the url is giving a 404 not found because its been removed

[jedieaston]

I've actually brought this up earlier, the Locale Schema does actually not say that Moniker is a invalid variable

The REST schema does, maybe the JSON schema does not? I wrote a implementation at one point of most of the REST documentation and that was one of the things that surprised me.

[OfficialEsco]

I've actually brought this up earlier, the Locale Schema does actually not say that Moniker is a invalid variable

The REST schema does, maybe the JSON schema does not? I wrote a implementation at one point of most of the REST documentation and that was one of the things that surprised me.

What is REST? And where is it?
Moniker is valid in the JSON which YamlCreate.ps1 uses to find all the possible variables, either this have to be removed from the JSON or you need to point us to a readable REST Schema

@Trenly needs to add support for defaultLocale tho, i think he used Locale because there was no difference between defaultLocale and Locale

[jedieaston]

Here's the REST schema, used for defining what a REST source's parameters/responses are (It's a OpenAPI document). I don't know why it's not in the winget-cli repo.

Line 1144 in that file shows that defaultLocale extends Locale, adding a single field, Moniker.

I don't think that we should really be referencing this over the JSON schemas, but I think that since JSON schemas don't typically define a strict set of the only keys that can exist (while OpenAPI docs do), it will be difficult to automatically validate using just the JSON schema.

[Trenly]

@vedantmgoyal2009 I've updated the minimum length
@jedieaston I've updated the user agent
@OfficialEsco I've opened two issues related to moniker for the 2.0 version; The first issue is to add the defaultLocale schema as a separate entity from the locale schema, and the second is to fix the issue with the moniker

[Trenly]

I've actually brought this up earlier, the Locale Schema does actually not say that Moniker is a invalid variable

However winget validate and the bot have always showed a warning for it

Have you ever created an issue for this? I'd like to look at the discussions surrounding it. If possible, I think we should try and get the JSON schema updated

[jedieaston]

I don’t know if there’s an issue, but it looks like additionalProperties needs to be set to false for the JSON Schema to enforce random keys not being there.

[Trenly]

I don’t know if there’s an issue, but it looks like additionalProperties needs to be set to false for the JSON Schema to enforce random keys not being there.

I know that, but what if someone has reason to add keys that aren't in the schema? If I'm not mistaken, Esco specifically said these keys should be kept - microsoft#21204 (review)

[jedieaston]

They shouldn’t IMO, given they can’t be used in the client. I think those fields were added in v1.1, but if they are only in 1.1 then they shouldn’t be used in 1.0.

@OfficialEsco I don’t understand the reasoning behind adding those fields, can you explain?

[OfficialEsco]

I only work with fields that are in the current JSON Schema, all our fields are in the Schema. If they don't work in the current client but is in the Schema and the bot approved them then that is not our problem, the client will get support for it and those fields will be filled.

[Trenly]

Lets leave that discussion for a separate issue; My main concern with this PR is to ensure the files download successfully