From 821b98e26cc153a2806aff79033ce6b52249ee87 Mon Sep 17 00:00:00 2001 From: Natalie Weizenbaum Date: Thu, 11 Apr 2024 15:51:31 -0700 Subject: [PATCH] Don't cache canonicalize calls when `containingUrl` is available (#2215) See #2208 --- CHANGELOG.md | 10 ++++ lib/src/async_import_cache.dart | 85 ++++++++++++++++++++------------- lib/src/import_cache.dart | 84 ++++++++++++++++++++------------ pkg/sass_api/CHANGELOG.md | 4 ++ pkg/sass_api/pubspec.yaml | 4 +- pubspec.yaml | 2 +- 6 files changed, 124 insertions(+), 65 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1979177f2..ae92aca14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,13 @@ +## 1.75.0 + +* Fix a bug in which stylesheet canonicalization could be cached incorrectly + when custom importers or the Node.js package importer made decisions based on + the URL of the containing stylesheet. + +### JS API + +* Allow `importer` to be passed without `url` in `StringOptionsWithImporter`. + ## 1.74.1 * No user-visible changes. diff --git a/lib/src/async_import_cache.dart b/lib/src/async_import_cache.dart index 0deb6285f..576094cb4 100644 --- a/lib/src/async_import_cache.dart +++ b/lib/src/async_import_cache.dart @@ -154,64 +154,85 @@ final class AsyncImportCache { } if (baseImporter != null && url.scheme == '') { - var relativeResult = await putIfAbsentAsync( - _relativeCanonicalizeCache, - ( - url, - forImport: forImport, - baseImporter: baseImporter, - baseUrl: baseUrl - ), - () => _canonicalize(baseImporter, baseUrl?.resolveUri(url) ?? url, - baseUrl, forImport)); + var relativeResult = await putIfAbsentAsync(_relativeCanonicalizeCache, ( + url, + forImport: forImport, + baseImporter: baseImporter, + baseUrl: baseUrl + ), () async { + var (result, cacheable) = await _canonicalize( + baseImporter, baseUrl?.resolveUri(url) ?? url, baseUrl, forImport); + assert( + cacheable, + "Relative loads should always be cacheable because they never " + "provide access to the containing URL."); + return result; + }); if (relativeResult != null) return relativeResult; } - return await putIfAbsentAsync( - _canonicalizeCache, (url, forImport: forImport), () async { - for (var importer in _importers) { - if (await _canonicalize(importer, url, baseUrl, forImport) - case var result?) { + var key = (url, forImport: forImport); + if (_canonicalizeCache.containsKey(key)) return _canonicalizeCache[key]; + + // Each indivudal call to a `canonicalize()` override may not be cacheable + // (specifically, if it has access to `containingUrl` it's too + // context-sensitive to usefully cache). We want to cache a given URL across + // the _entire_ importer chain, so we use [cacheable] to track whether _all_ + // `canonicalize()` calls we've attempted are cacheable. Only if they are do + // we store the result in the cache. + var cacheable = true; + for (var importer in _importers) { + switch (await _canonicalize(importer, url, baseUrl, forImport)) { + case (var result?, true) when cacheable: + _canonicalizeCache[key] = result; + return result; + + case (var result?, _): return result; - } + + case (_, false): + cacheable = false; } + } - return null; - }); + if (cacheable) _canonicalizeCache[key] = null; + return null; } /// Calls [importer.canonicalize] and prints a deprecation warning if it /// returns a relative URL. /// - /// If [resolveUrl] is `true`, this resolves [url] relative to [baseUrl] - /// before passing it to [importer]. - Future _canonicalize( - AsyncImporter importer, Uri url, Uri? baseUrl, bool forImport, - {bool resolveUrl = false}) async { - var resolved = - resolveUrl && baseUrl != null ? baseUrl.resolveUri(url) : url; + /// This returns both the result of the call to `canonicalize()` and whether + /// that result is cacheable at all. + Future<(AsyncCanonicalizeResult?, bool cacheable)> _canonicalize( + AsyncImporter importer, Uri url, Uri? baseUrl, bool forImport) async { var canonicalize = forImport - ? () => inImportRule(() => importer.canonicalize(resolved)) - : () => importer.canonicalize(resolved); + ? () => inImportRule(() => importer.canonicalize(url)) + : () => importer.canonicalize(url); var passContainingUrl = baseUrl != null && (url.scheme == '' || await importer.isNonCanonicalScheme(url.scheme)); var result = await withContainingUrl( passContainingUrl ? baseUrl : null, canonicalize); - if (result == null) return null; + + // TODO(sass/dart-sass#2208): Determine whether the containing URL was + // _actually_ accessed rather than assuming it was. + var cacheable = !passContainingUrl || importer is FilesystemImporter; + + if (result == null) return (null, cacheable); if (result.scheme == '') { _logger.warnForDeprecation( Deprecation.relativeCanonical, - "Importer $importer canonicalized $resolved to $result.\n" + "Importer $importer canonicalized $url to $result.\n" "Relative canonical URLs are deprecated and will eventually be " "disallowed."); } else if (await importer.isNonCanonicalScheme(result.scheme)) { - throw "Importer $importer canonicalized $resolved to $result, which " - "uses a scheme declared as non-canonical."; + throw "Importer $importer canonicalized $url to $result, which uses a " + "scheme declared as non-canonical."; } - return (importer, result, originalUrl: resolved); + return ((importer, result, originalUrl: url), cacheable); } /// Tries to import [url] using one of this cache's importers. diff --git a/lib/src/import_cache.dart b/lib/src/import_cache.dart index e34f0a7ee..6204971e4 100644 --- a/lib/src/import_cache.dart +++ b/lib/src/import_cache.dart @@ -5,7 +5,7 @@ // DO NOT EDIT. This file was generated from async_import_cache.dart. // See tool/grind/synchronize.dart for details. // -// Checksum: d157b83599dbc07a80ac6cb5ffdf5dde03b60376 +// Checksum: 37dd173d676ec6cf201a25b3cca9ac81d92b1433 // // ignore_for_file: unused_import @@ -154,61 +154,85 @@ final class ImportCache { } if (baseImporter != null && url.scheme == '') { - var relativeResult = _relativeCanonicalizeCache.putIfAbsent( - ( - url, - forImport: forImport, - baseImporter: baseImporter, - baseUrl: baseUrl - ), - () => _canonicalize(baseImporter, baseUrl?.resolveUri(url) ?? url, - baseUrl, forImport)); + var relativeResult = _relativeCanonicalizeCache.putIfAbsent(( + url, + forImport: forImport, + baseImporter: baseImporter, + baseUrl: baseUrl + ), () { + var (result, cacheable) = _canonicalize( + baseImporter, baseUrl?.resolveUri(url) ?? url, baseUrl, forImport); + assert( + cacheable, + "Relative loads should always be cacheable because they never " + "provide access to the containing URL."); + return result; + }); if (relativeResult != null) return relativeResult; } - return _canonicalizeCache.putIfAbsent((url, forImport: forImport), () { - for (var importer in _importers) { - if (_canonicalize(importer, url, baseUrl, forImport) case var result?) { + var key = (url, forImport: forImport); + if (_canonicalizeCache.containsKey(key)) return _canonicalizeCache[key]; + + // Each indivudal call to a `canonicalize()` override may not be cacheable + // (specifically, if it has access to `containingUrl` it's too + // context-sensitive to usefully cache). We want to cache a given URL across + // the _entire_ importer chain, so we use [cacheable] to track whether _all_ + // `canonicalize()` calls we've attempted are cacheable. Only if they are do + // we store the result in the cache. + var cacheable = true; + for (var importer in _importers) { + switch (_canonicalize(importer, url, baseUrl, forImport)) { + case (var result?, true) when cacheable: + _canonicalizeCache[key] = result; + return result; + + case (var result?, _): return result; - } + + case (_, false): + cacheable = false; } + } - return null; - }); + if (cacheable) _canonicalizeCache[key] = null; + return null; } /// Calls [importer.canonicalize] and prints a deprecation warning if it /// returns a relative URL. /// - /// If [resolveUrl] is `true`, this resolves [url] relative to [baseUrl] - /// before passing it to [importer]. - CanonicalizeResult? _canonicalize( - Importer importer, Uri url, Uri? baseUrl, bool forImport, - {bool resolveUrl = false}) { - var resolved = - resolveUrl && baseUrl != null ? baseUrl.resolveUri(url) : url; + /// This returns both the result of the call to `canonicalize()` and whether + /// that result is cacheable at all. + (CanonicalizeResult?, bool cacheable) _canonicalize( + Importer importer, Uri url, Uri? baseUrl, bool forImport) { var canonicalize = forImport - ? () => inImportRule(() => importer.canonicalize(resolved)) - : () => importer.canonicalize(resolved); + ? () => inImportRule(() => importer.canonicalize(url)) + : () => importer.canonicalize(url); var passContainingUrl = baseUrl != null && (url.scheme == '' || importer.isNonCanonicalScheme(url.scheme)); var result = withContainingUrl(passContainingUrl ? baseUrl : null, canonicalize); - if (result == null) return null; + + // TODO(sass/dart-sass#2208): Determine whether the containing URL was + // _actually_ accessed rather than assuming it was. + var cacheable = !passContainingUrl || importer is FilesystemImporter; + + if (result == null) return (null, cacheable); if (result.scheme == '') { _logger.warnForDeprecation( Deprecation.relativeCanonical, - "Importer $importer canonicalized $resolved to $result.\n" + "Importer $importer canonicalized $url to $result.\n" "Relative canonical URLs are deprecated and will eventually be " "disallowed."); } else if (importer.isNonCanonicalScheme(result.scheme)) { - throw "Importer $importer canonicalized $resolved to $result, which " - "uses a scheme declared as non-canonical."; + throw "Importer $importer canonicalized $url to $result, which uses a " + "scheme declared as non-canonical."; } - return (importer, result, originalUrl: resolved); + return ((importer, result, originalUrl: url), cacheable); } /// Tries to import [url] using one of this cache's importers. diff --git a/pkg/sass_api/CHANGELOG.md b/pkg/sass_api/CHANGELOG.md index 304ec3549..77d3aaa74 100644 --- a/pkg/sass_api/CHANGELOG.md +++ b/pkg/sass_api/CHANGELOG.md @@ -1,3 +1,7 @@ +## 10.2.0 + +* No user-visible changes. + ## 10.1.1 * No user-visible changes. diff --git a/pkg/sass_api/pubspec.yaml b/pkg/sass_api/pubspec.yaml index 1adf7da2c..ff9a9b383 100644 --- a/pkg/sass_api/pubspec.yaml +++ b/pkg/sass_api/pubspec.yaml @@ -2,7 +2,7 @@ name: sass_api # Note: Every time we add a new Sass AST node, we need to bump the *major* # version because it's a breaking change for anyone who's implementing the # visitor interface(s). -version: 10.1.1 +version: 10.2.0 description: Additional APIs for Dart Sass. homepage: https://github.com/sass/dart-sass @@ -10,7 +10,7 @@ environment: sdk: ">=3.0.0 <4.0.0" dependencies: - sass: 1.74.1 + sass: 1.75.0 dev_dependencies: dartdoc: ^6.0.0 diff --git a/pubspec.yaml b/pubspec.yaml index e37160085..54602aa9a 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -1,5 +1,5 @@ name: sass -version: 1.74.1 +version: 1.75.0 description: A Sass implementation in Dart. homepage: https://github.com/sass/dart-sass