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

AndroidAnnotationSupport needs better invalid XML fixup #883

Closed
jonpryor opened this issue Sep 20, 2021 · 0 comments · Fixed by #897
Closed

AndroidAnnotationSupport needs better invalid XML fixup #883

jonpryor opened this issue Sep 20, 2021 · 0 comments · Fixed by #897
Assignees
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)

Comments

@jonpryor
Copy link
Member

Context: https://issuetracker.google.com/issues/116182838
Context: 4073f3e

In 2018-September, Google started providing various XML files which were not valid XML. We worked around this in commit 4073f3e by using HtmlAgilityPack to "loosely parse" the XML.

The problem with commit 4073f3e is that the resulting "intermediate" XML isn't "the same". Given XML of:

  <!-- valid XML; API-28 -->
   <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;, android.os.Handler)">
     <annotation name="android.support.annotation.RequiresPermission">
      <val name="value" val="&quot;android.permission.MANAGE_ACCOUNTS&quot;" />
       <val name="apis" val="&quot;..22&quot;" />
     </annotation>
   </item>

    <!-- invalid XML; API-29 -->
   <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">
     <annotation name="androidx.annotation.RequiresPermission">
       <val name="value" val="&quot;android.permission.MANAGE_ACCOUNTS&quot;" />
       <val name="apis" val="&quot;..22&quot;" />
     </annotation>
   </item>

Consider this fragment:

using System;
using System.Collections.Generic;
using System.Linq;
using Xamarin.AndroidTools.AnnotationSupport;

class App {
    public static void Main ()
    {
        Console.WriteLine ("- API-28");
        var a28 = AndroidAnnotationsSupport.ParseArchive ("$HOME/android-toolchain/sdk/platforms/android-28/data/annotations.zip");
        Console.WriteLine ("- API-29");
        var a29 = AndroidAnnotationsSupport.ParseArchive ("$HOME/android-toolchain/sdk/platforms/android-29/data/annotations.zip");

        var addAccount_28 = a28.Where (a => a.TypeName == "android.accounts.AccountManager" && a.MemberName == "addAccount")
            .First ();
        var addAccount_29 = a29.Where (a => a.TypeName == "android.accounts.AccountManager" && a.MemberName == "addAccount")
            .First ();
        
        Console.WriteLine ($"API-28: {addAccount_28}");
        foreach (var a in addAccount_28.Annotations) {
            Console.WriteLine ($" Name={a.Name}");
            var v = string.Join (", ", a.Values);
            Console.WriteLine ($" Values={v}");
        }
        
        Console.WriteLine ($"API-29: {addAccount_29}");
        foreach (var a in addAccount_29.Annotations) {
            Console.WriteLine ($" Name={a.Name}");
            var v = string.Join (", ", a.Values);
            Console.WriteLine ($" Values={v}");
        }
    }
}

When run, we get:

API-28: @RequiresPermission(value=System.String[], apis=System.String[]) android.accounts.AccountManager.addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)
 Name=RequiresPermission
 Values=value = "android.permission.MANAGE_ACCOUNTS", apis = "..22"
API-29: @RequiresPermission(value=System.String[], apis=System.String[]) android.accounts.AccountManager.addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)
 Name=RequiresPermission
 Values=value = &quot;android.permission.MANAGE_ACCOUNTS&quot;, apis = &quot;..22&quot;

Note how the Values value changes, from android.permission.MANAGE_ACCOUNTS to &quot;android.permission.MANAGE_ACCOUNTS&quot. This value eventually becomes the RequiresPermission attribute value, resulting in "downstream" Mono.Android.dll changes:

-[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
+[global::Android.Runtime.RequiresPermission ("quot;android.permission.MANAGE_ACCOUNTS&quot")]

which results in hundreds of "API changes" reported by the _CheckApiCompatibility target.

For Great Sanity™, we want the "invalid XML" path to produce the same value as the "valid XML" path, especially considering that the //item/annotation/val/@val attribute value is the same.

jonpryor added a commit to jonpryor/xamarin-android that referenced this issue Sep 20, 2021
Context: dotnet#6300
Context: dotnet/java-interop#883

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (dotnet#134)

Note: while this xamarin-android-tools bump updates the Android SDK
[Platform Tools][0] version to 31.0.3, we are *not* updating
`$(XAPlatformToolsVersion)` to 31.0.3.  As such, there will be a
mismatch between what we build xamarin-android against, vs. the
default suggested platform-tools package potentially installed
via the `/t:InstallAndroidDependencies` target.

The reason to *not* bump `$(XAPlatformToolsVersion)` is that
attempting to do so breaks the `src/Mono.Android` build:

 1. platform-tools r31.0.3 doesn't contain
    `platform-tools/api/annotations.zip`, which is used by
    `generator` to emit certain custom attributes such as
    `RequiresPermission`.

 2. While (1) can be worked around by instead using
    `$(AndroidSdkDirectory)/platforms/android-*/data/annotations.zip`,
    this introduces API changes reported by the
    `_CheckApiCompatibility` target, in particular changes due to
    custom attribute string changes.  These string changes happen
    because Google ships *invalid XML* in `data/annotations.zip` for
    API-29+ (?!):

        <!-- annotations.zip!android/accounts/annotations.xml -->
        -  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;, android.os.Handler)">
        +  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">

    `AccountManagerCallback<android.os.Bundle>` is not a valid value
    within an XML attribute.

    This change doesn't break the build, but instead causes
    [`AndroidAnnotationSupport`][1] to hit a "fallback" codepath
    which uses HtmlAgilityPack, and this causes attribute values
    to "gain" `quot;`:

        -[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
        +[global::Android.Runtime.RequiresPermission ("quot;android.permission.MANAGE_ACCOUNTS&quot")]

    See also: dotnet/java-interop#883

After manual review, no other file removals appear to be problematic,
so @jonpryor asserts that it should be acceptable for
`$(XAPlatformToolsVersion)` and `$(AndroidSdkPlatformToolsVersion)`
to be inconsistent with each other.

[0]: https://developer.android.com/studio/releases/platform-tools
[1]: https://github.com/xamarin/java.interop/blob/1e8f5137345db3160c99265ff3a56c43a132194f/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs#L58-L87
@jonpryor jonpryor self-assigned this Sep 20, 2021
jonpryor added a commit to dotnet/android that referenced this issue Sep 20, 2021
Context: #6300
Context: dotnet/java-interop#883

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134)

Note: while this xamarin-android-tools bump updates the Android SDK
[Platform Tools][0] version to 31.0.3, we are *not* updating
`$(XAPlatformToolsVersion)` to 31.0.3.  As such, there will be a
mismatch between what we build xamarin-android against, vs. the
default suggested platform-tools package potentially installed
via the `/t:InstallAndroidDependencies` target.

The reason to *not* bump `$(XAPlatformToolsVersion)` is that
attempting to do so breaks the `src/Mono.Android` build:

 1. platform-tools r31.0.3 doesn't contain
    `platform-tools/api/annotations.zip`, which is used by
    `generator` to emit certain custom attributes such as
    `RequiresPermission`.

 2. While (1) can be worked around by instead using
    `$(AndroidSdkDirectory)/platforms/android-*/data/annotations.zip`,
    this introduces API changes reported by the
    `_CheckApiCompatibility` target, in particular changes due to
    custom attribute string changes.  These string changes happen
    because Google ships *invalid XML* in `data/annotations.zip` for
    API-29+ (?!):

        <!-- annotations.zip!android/accounts/annotations.xml -->
        -  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;, android.os.Handler)">
        +  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">

    `AccountManagerCallback<android.os.Bundle>` is not a valid value
    within an XML attribute.

    This change doesn't break the build, but instead causes
    [`AndroidAnnotationSupport`][1] to hit a "fallback" codepath
    which uses HtmlAgilityPack, and this causes attribute values
    to "gain" `quot;`:

        -[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
        +[global::Android.Runtime.RequiresPermission ("quot;android.permission.MANAGE_ACCOUNTS&quot")]

    See also: dotnet/java-interop#883

After manual review, no other file removals appear to be problematic,
so @jonpryor asserts that it should be acceptable for
`$(XAPlatformToolsVersion)` and `$(AndroidSdkPlatformToolsVersion)`
to be inconsistent with each other.

[0]: https://developer.android.com/studio/releases/platform-tools
[1]: https://github.com/xamarin/java.interop/blob/1e8f5137345db3160c99265ff3a56c43a132194f/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs#L58-L87
jonathanpeppers pushed a commit to dotnet/android that referenced this issue Sep 21, 2021
Context: #6300
Context: dotnet/java-interop#883

Changes: http://github.com/xamarin/xamarin-android-tools/compare/9b658b29bd41157151f5515619d0d90dc062563d...a5194e93498e7f12225d87e2811415a45f742116

  * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30
  * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134)

Note: while this xamarin-android-tools bump updates the Android SDK
[Platform Tools][0] version to 31.0.3, we are *not* updating
`$(XAPlatformToolsVersion)` to 31.0.3.  As such, there will be a
mismatch between what we build xamarin-android against, vs. the
default suggested platform-tools package potentially installed
via the `/t:InstallAndroidDependencies` target.

The reason to *not* bump `$(XAPlatformToolsVersion)` is that
attempting to do so breaks the `src/Mono.Android` build:

 1. platform-tools r31.0.3 doesn't contain
    `platform-tools/api/annotations.zip`, which is used by
    `generator` to emit certain custom attributes such as
    `RequiresPermission`.

 2. While (1) can be worked around by instead using
    `$(AndroidSdkDirectory)/platforms/android-*/data/annotations.zip`,
    this introduces API changes reported by the
    `_CheckApiCompatibility` target, in particular changes due to
    custom attribute string changes.  These string changes happen
    because Google ships *invalid XML* in `data/annotations.zip` for
    API-29+ (?!):

        <!-- annotations.zip!android/accounts/annotations.xml -->
        -  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback&lt;android.os.Bundle&gt;, android.os.Handler)">
        +  <item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">

    `AccountManagerCallback<android.os.Bundle>` is not a valid value
    within an XML attribute.

    This change doesn't break the build, but instead causes
    [`AndroidAnnotationSupport`][1] to hit a "fallback" codepath
    which uses HtmlAgilityPack, and this causes attribute values
    to "gain" `quot;`:

        -[global::Android.Runtime.RequiresPermission ("android.permission.MANAGE_ACCOUNTS")]
        +[global::Android.Runtime.RequiresPermission ("quot;android.permission.MANAGE_ACCOUNTS&quot")]

    See also: dotnet/java-interop#883

After manual review, no other file removals appear to be problematic,
so @jonpryor asserts that it should be acceptable for
`$(XAPlatformToolsVersion)` and `$(AndroidSdkPlatformToolsVersion)`
to be inconsistent with each other.

[0]: https://developer.android.com/studio/releases/platform-tools
[1]: https://github.com/xamarin/java.interop/blob/1e8f5137345db3160c99265ff3a56c43a132194f/src/Xamarin.Android.Tools.AnnotationSupport/AndroidAnnotationsSupport.cs#L58-L87
@jpobst jpobst added enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.) labels Sep 27, 2021
jonpryor pushed a commit that referenced this issue Oct 26, 2021
Fixes: #883

Due to invalid XML provided in Google's `annotations.zip` file, we
run it through the more forgiving `HtmlAgilityPack` (4073f3e) to
attempt to fix it to valid XML.

However, given this snippet:

	<item name="android.accounts.AccountManager android.accounts.AccountManagerFuture&lt;android.os.Bundle&gt; addAccount(java.lang.String, java.lang.String, java.lang.String[], android.os.Bundle, android.app.Activity, android.accounts.AccountManagerCallback<android.os.Bundle>, android.os.Handler)">
	  <annotation name="androidx.annotation.RequiresPermission">
	    <val name="value" val="&quot;android.permission.MANAGE_ACCOUNTS&quot;" />
	    <val name="apis" val="&quot;..22&quot;" />
	  </annotation>
	</item>

The invalid unescaped `<` and `>` characters in the `//item/@name`
attribute seem to tell `HtmlAgilityPack` not to expect any attribute
strings to be properly escaped.  Thus when it gets to the `//val/@val`
attributes, it treats `&quot;` as unescaped as well, thinking we want
to value to be the literal string `"&quot;"`.

When it writes out the valid XML, it realizes it needs to escape the
ampersand, and writes out `&amp;quot;`, which breaks our usage of
these annotations.

The fix is to "unescape" every `&quot;` to a `"` so that it will be
escaped correctly when saved as valid XML.
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Proposed change to current functionality generator Issues binding a Java library (generator, class-parse, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants