Catch all bundletool exceptions when matching device APKs

[thomasbuilds]

GetAppDownloadInfo and GetAppUpdateInfo pass a fully client-controlled device spec to bundletool's ApkMatcher. Only the spec's presence is validated (the appstore-api DeviceAttributes.spec field is required, but its contents are not), so arbitrary device specs reach bundletool.

getMatchingApkPaths only caught IncompatibleDeviceException, but bundletool rejects specs it cannot interpret with other sibling BundleToolException subtypes. For example, an app whose bundle has multi-ABI variant targeting causes MultiAbiMatcher to throw an InvalidCommandException for an unrecognized ABI in the device spec. Those escaped uncaught instead of being treated as an incompatible device.

Catch the common BundleToolException base instead so any device spec bundletool cannot match or interpret yields an empty match set.

[lberrymage]

Hmm. If the DeviceSpec is invalid, I'm wondering if we should treat that as an invalid request instead of an incompatible device.

We already have some protovalidate rules on our Buf-packaged version of bundletool's protobuf definitions. We could add to those (or the app store API protos) and get further validation basically for free, but I haven't mapped out all of the possible exceptions bundletool could throw through ApkMatcher and why, so that approach might not have accurate semantics.

Overall I'm thinking:

• If we're making a change like this, I want to make sure it's complete. There appear to be paths to other uncovered exceptions, e.g., IllegalArgumentException at https://github.com/google/bundletool/blob/586a43a450712a1067f3d92cf7574dee68226302/src/main/java/com/android/tools/build/bundletool/device/SdkVersionMatcher.java#L54-L56 when ApkMatcher is passed an "invalid" DeviceSpec.
• If the remaining uncovered exceptions are similar to what I've seen so far, I think INVALID_ARGUMENT is a more appropriate status code than FAILED_PRECONDITION.
• If we go with INVALID_ARGUMENT, adding protovalidate annotations to appstore-api on DeviceSpec fields is the most straightforward implementation.

What do you think?

Also FYI, the directory server is being subsumed by Parcelo, so changes to it fit better there since that's where active development is happening. This change is fine here, but I'll eventually copy it over to Parcelo anyway.

[thomasbuilds]

Yeah, I think an invalid spec should be INVALID_ARGUMENT. The one case I'd keep separate is a genuinely incompatible device, where the spec is fine but nothing matches, since that still feels like FAILED_PRECONDITION.

I'd lean toward validation over a catch because the exception type doesn't reliably tell those two apart. Running a few specs through ApkMatcher, the same unrecognized ABI comes back as an IncompatibleDeviceException on a split app (or gets silently ignored there if a real ABI the app has a split for comes before it in the list) but an InvalidCommandException on a multi-ABI one. So mapping the catch by exception type would give the same bad spec different codes depending on the app.

On completeness, the SdkVersionMatcher checkArgument you linked is on the variant's SDK targeting rather than the device spec (the message even says "in the variant targeting"), so it comes from a malformed BuildApksResult, not a client DeviceSpec. But your worry holds elsewhere: DeviceTierApkMatcher and CountrySetApkMatcher both checkArgument straight on the client's device_tier / country_set, so for an app that uses those a mismatched value comes out as a plain IllegalArgumentException, not a BundleToolException. Those are matched against each bundle's available values, so protovalidate can't cover them, and we'd still want a catch as a backstop for those, wider than BundleToolException.

For the implementation I think you're right that protovalidate is the way to go for the ABI case. The existing DeviceSpec rules and validateDeviceSpec only check that the lists are non-empty and sdk/density are positive, so we'd add something like restricting supported_abis to the known ABI names. The one tradeoff is it pins the set, so a brand new ABI bundletool doesn't recognize yet would read as INVALID_ARGUMENT rather than just incompatible.

[thomasbuilds]

For the time being I've implemented the supported_abis check in code rather than as a protovalidate rule, since the rules live in android-bundle / appstore-api