Ensure MacAppStoreDumper only dumps valid app details
#349
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jacobbednarz
force-pushed
the
address-mas-invalid-output
branch
2 times, most recently
from
fa26194 to
9051692
Compare
jacobbednarz
commented
Jul 19, 2018
| end | ||
|
|
||
| it "dumps excluding invalid apps" do | ||
| expect(subject.dump).to eq(expected_mas_dumped_output.strip) |
There was a problem hiding this comment.
The strip here is an unfortunate side effect of the HEREDOCs and having a \n on the end. I tried various combinations of %Q/%q/escaping and nothing seemed to be as readable as the HEREDOC so I thought this was a worthy trade off.
During a routine `brew bundle` I came across the following exception (which manages to halt all commands): ``` Error: Invalid Brewfile: uninitialized constant #<Class:#<Bundle::Dsl:0x00000101a5d898>>::Install ``` Searching around the issue tracker, I noticed a couple of existing[1] issues[2] where this was occurring when the `mas list` output included an application from the app store that wasn't formatted as `brew bundle` expected. Sure enough, this was also my issue. ``` 497799835 Xcode (9.2) 409183694 Keynote (8.1) 408981434 iMovie (10.1.8) Install macOS High Sierra (13105) 409201541 Pages (7.1) 682658836 GarageBand (10.3.1) 409203825 Numbers (5.1) ``` Taking a look at the `MacAppStoreDumper` class, the way this output was previously generated was by splitting on the first bit of whitespace it encounters and assigned the two parts to the app ID and app name respectively. In the case of the High Sierra upgrade app, there wasn't an ID for it and the first bit of whitespace came after "Install". This explains why the generated line would be: ``` mas "macOS High Sierra", id: Install ``` It was splitting on the first bit of whitespace it encountered and didn't restrict what that first value could possibly be for it to be a valid application. In order to firm up the app details extraction method a little more, I've swapped the extraction to use a regular expression with named captures. By doing this, it introduces a (very) loose validation of what the `mas list` output should be providing. If the output format changes, it won't break `brew bundle` subcommands, it will just fail to add them to the generated output. Additionally, there is a new `compact` call added to the `@apps` value to ensure that the `dump` method doesn't need to concern itself with iterating on empty values and potentially throwing `nilClass` exceptions for the `downcase` name calls. Fixes Homebrew#308 Fixes Homebrew#321 [1]: Homebrew#321 [2]: Homebrew#308
The output that was being asserted against wasn't correct and lead to it failing the validation included in b967c1c. In order to assert against the actual output, the version number and ID has been added.
jacobbednarz
force-pushed
the
address-mas-invalid-output
branch
from
9051692 to
3100df5
Compare
jacobbednarz
commented
Jul 19, 2018
| 497799835 Xcode (9.2) | ||
| 425424353 The Unarchiver (4.0.0) | ||
| 08981434 iMovie (10.1.8) | ||
| Install macOS High Sierra (13105) |
There was a problem hiding this comment.
This space is important to ensure it matches the mas list output.
|
A perfect PR and amazing first time contribution. You really rock @jacobbednarz, nice work! |
jacobbednarz
added a commit
to jacobbednarz/homebrew-bundle
that referenced
this pull request
Aug 27, 2018
Homebrew#349 introduced a basic regex for determining if the app names provided via `mas` were valid and would discard any that failed to meet this requirement. Apple are quite generous[1] when it comes to what you can name an application and in essence, the only restriction they place on it is that it cannot infringe on a trademark and must be under 30 characters long. This means that there are real applications that fail to meet the initial regex of `\w\s` due to using non-alphanumeric characters. "Pastebin It!" is an example of this. To account for this, I've extended the regex to accept any character for the `name` portion of the regex. This might look fragile (and I would agree!) however it is the safest way to ensure that we don't silently drop applications from being included in the output. Should Apple tighten this restriction in the future, we can firm it up with a better regex. Fixes Homebrew#359 [1]: https://developer.apple.com/app-store/review/guidelines/#accurate-metadata
jacobbednarz
added a commit
to jacobbednarz/homebrew-bundle
that referenced
this pull request
Aug 28, 2018
Homebrew#349 introduced a basic regex for determining if the app names provided via `mas` were valid and would discard any that failed to meet this requirement. Apple are quite generous[1] when it comes to what you can name an application and in essence, the only restriction they place on it is that it cannot infringe on a trademark and must be under 30 characters long. This means that there are real applications that fail to meet the initial regex of `\w\s` due to using non-alphanumeric characters. "Pastebin It!" is an example of this. To account for this, I've extended the regex to accept any character for the `name` portion of the regex. This might look fragile (and I would agree!) however it is the safest way to ensure that we don't silently drop applications from being included in the output. Should Apple tighten this restriction in the future, we can firm it up with a better regex. Fixes Homebrew#359 [1]: https://developer.apple.com/app-store/review/guidelines/#accurate-metadata
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
During a routine
brew bundleI came across the following exception(which manages to halt all commands):
Searching around the issue tracker, I noticed a couple of existing
issues where this was occurring when the
mas listoutput includedan application from the app store that wasn't formatted as
brew bundleexpected. Sure enough, this was also my issue.
Taking a look at the
MacAppStoreDumperclass, the way this output waspreviously generated was by splitting on the first bit of whitespace it
encounters and assigned the two parts to the app ID and app name
respectively. In the case of the High Sierra upgrade app, there wasn't
an ID for it and the first bit of whitespace came after "Install". This
explains why the generated line would be:
It was splitting on the first bit of whitespace it encountered and
didn't restrict what that first value could possibly be for it to be a
valid application.
In order to firm up the app details extraction method a little more,
I've swapped the extraction to use a regular expression with named
captures. By doing this, it introduces a (very) loose validation of
what the
mas listoutput should be providing. If the output formatchanges, it won't break
brew bundlesubcommands, it will just fail toadd them to the generated output.
Additionally, there is a new
compactcall added to the@appsvalueto ensure that the
dumpmethod doesn't need to concern itself withiterating on empty values and potentially throwing
nilClassexceptionsfor the
downcasename calls.Fixes #308
Fixes #321