Implement loadStructuredBinaryData from updated AssetBundle

[arnemolland]

📜 Description

Implements loadStructuredBinaryData, which is a new method added to Flutter's abstract AssetBundle class in flutter/flutter#119277.

💡 Motivation and Context

This change has to be included in order to support what is now the master channel of Flutter and what will hit stable down the road.

💚 How did you test it?

Duped and tweaked the tests for loadStructuredData.

📝 Checklist

• I reviewed submitted code
• I added tests to verify changes
• No new PII added or SDK only sends newly added PII if sendDefaultPii is enabled
• I updated the docs if needed
• All tests passing
• No breaking changes

🔮 Next steps

[marandaneto]

Thank you @arnemolland
We'll have to wait for this to land at least on the beta channel, otherwise, it'd be breaking for people using the stable/beta channels.

[ueman]

It can be made backwards compatible as shown in #1166 (comment)

[arnemolland]

It can be made backwards compatible as shown in #1166 (comment)

Updated the PR, would you prefer not using the inner asset bundle?

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.06 🎉

Comparison is base (d301b11) 89.76% compared to head (0b1a611) 89.83%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1272      +/-   ##
==========================================
+ Coverage   89.76%   89.83%   +0.06%     
==========================================
  Files         155      155              
  Lines        5032     5064      +32     
==========================================
+ Hits         4517     4549      +32     
  Misses        515      515              

Impacted Files	Coverage Δ
flutter/lib/src/sentry_asset_bundle.dart	94.91% <100.00%> (+1.89%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[ueman]

This PR is currently failing because bundle.loadStructuredBinaryData isn't available on the min Flutter version which Sentry supports. Therefore, we need to apply the suggested "hack" to make it compatible with the min as well as the max Flutter versions supported by Sentry.

You also need to add a changelog entry.

This PR fixes:

• sentry_asset_bundle uses loadStructuredBinaryData which has a breaking change in latest master channel #1166
• Latest version is not compatible with Flutter 3.7 #1289
• SentryAssetBundle implementation is missing #1286
• Issue after upgrading to Flutter 3.8.0-10.1.pre #1293

(They're marked as completed because of this PR, I guess)

[arnemolland]

Thanks! Added the suggested changes.

[arnemolland]

Fixed an issue with type argument not being implicitly forwarded to the dynamic (bundle as dynamic).loadStructuredBinaryData call and rebased on top of the latest changes. Let me know if anything is missing.

[marandaneto]

@arnemolland since this is possible using the override_on_non_overriding_member and it's not a breaking change any more.
Can you target the main branch instead? so we can release this with the current stable version.

[arnemolland]

@arnemolland since this is possible using the override_on_non_overriding_member and it's not a breaking change any more.
Can you target the main branch instead? so we can release this with the current stable version.

Sure thing, I'll rebase

[marandaneto]

There are some conflicts, search for <<<<<<< HEAD.
Also, the changelog should be under the Unreleased (v6)

[arnemolland]

There are some conflicts, search for <<<<<<< HEAD.
Also, the changelog should be under the Unreleased (v6)

Had to tidy up locally, should be good now.

[marandaneto]

Thank you @arnemolland

[marandaneto]

@arnemolland there are 4 failing tests, mind checking out?

[arnemolland]

@arnemolland there are 4 failing tests, mind checking out?

Yes, seems to be related to tests running from stable trying to invoke the non-existing method, fixing that asap.

[arnemolland]

@arnemolland there are 4 failing tests, mind checking out?

Yes, seems to be related to tests running from stable trying to invoke the non-existing method, fixing that asap.

As expected, calling loadStructuredBinaryData on the inner bundle is no good for SDK versions <3.8. That wouldn't happen since it can't be called, but the test runner doesn't care since we're trying to call it directly instead of interfacing with an AssetBundle (as normal usage would be).

The quickest fix is to apply the second suggestion found here -- updating the PR.

[arnemolland]

My solution to the above problem was to refactor the code to emulate the AssetBundle's loadStructuredBinaryData implementation, so that we don't call the inner bundle and avoid the testing problem;

Future<T> _loadStructuredBinaryDataWrapper<T>(
    String key,
    FutureOr<T> Function(ByteData data) parser,
  ) async {
    final ByteData data = await load(key);
    return parser(data);
  }

This can be refactored back once the library drops backwards compatibility with Flutter < 3.8. LMK if this is an acceptable approach.

[ueman]

I don't think emulating is the way to go, as the loadStructuredBinaryData has quite a sophisticated implementation, which would be bypassed by emulating it. See https://master-api.flutter.dev/flutter/services/CachingAssetBundle/loadStructuredBinaryData.html
One particular drawback would be to basically remove the ability of the CachingAssetBundle to cache data for loadStructuredBinaryData.

Instead, you can adjust the TestAssetBundle and add loadStructuredBinaryData as a method just like in

sentry-dart/flutter/test/sentry_asset_bundle_test.dart

Lines 566 to 578 in 5e79e10

	@override
	// This is an override on Flutter greater than 3.1
	// ignore: override_on_non_overriding_member
	Future<ImmutableBuffer> loadBuffer(String key) async {
	if (throwException) {
	throw Exception('exception thrown for testing purposes');
	}
	if (key == _testFileName) {
	return ImmutableBuffer.fromUint8List(
	Uint8List.fromList(utf8.encode('Hello World!')));
	}
	return ImmutableBuffer.fromUint8List(Uint8List.fromList([]));
	}

Because the newly introduced method is then also available in all Flutter versions in the tests, every thing should be working.

I've done that in the past, see #877 as a reference how to do that.

[arnemolland]

I don't think emulating is the way to go, as the loadStructuredBinaryData has quite a sophisticated implementation, which would be bypassed by emulating it. See https://master-api.flutter.dev/flutter/services/CachingAssetBundle/loadStructuredBinaryData.html One particular drawback would be to basically remove the ability of the CachingAssetBundle to cache data for loadStructuredBinaryData.

Instead, you can adjust the TestAssetBundle and add loadStructuredBinaryData as a method just like in

sentry-dart/flutter/test/sentry_asset_bundle_test.dart

Lines 566 to 578 in 5e79e10

	@override
	// This is an override on Flutter greater than 3.1
	// ignore: override_on_non_overriding_member
	Future<ImmutableBuffer> loadBuffer(String key) async {
	if (throwException) {
	throw Exception('exception thrown for testing purposes');
	}
	if (key == _testFileName) {
	return ImmutableBuffer.fromUint8List(
	Uint8List.fromList(utf8.encode('Hello World!')));
	}
	return ImmutableBuffer.fromUint8List(Uint8List.fromList([]));
	}

Because the newly introduced method is then also available in all Flutter versions in the tests, every thing should be working.

I've done that in the past, see #877 as a reference how to do that.

Perfect, thanks!