From d037c082fbf9bd52971794991b5d1d48eb74951e Mon Sep 17 00:00:00 2001 From: Jacob MacDonald Date: Thu, 25 Jun 2015 08:47:28 -0700 Subject: [PATCH] fix(transformer): Don't hang on bad urls and log better errors closes https://github.com/angular/angular/issues/2605 --- .../src/transform/common/xhr_impl.dart | 7 ++- .../directive_processor/visitors.dart | 15 +++++- .../directive_processor/all_tests.dart | 48 ++++++++++++++++++- .../expected/hello.ng_deps.dart | 20 ++++++++ .../invalid_url_files/hello.dart | 13 +++++ 5 files changed, 99 insertions(+), 4 deletions(-) create mode 100644 modules/angular2/test/transform/directive_processor/invalid_url_files/expected/hello.ng_deps.dart create mode 100644 modules/angular2/test/transform/directive_processor/invalid_url_files/hello.dart diff --git a/modules/angular2/src/transform/common/xhr_impl.dart b/modules/angular2/src/transform/common/xhr_impl.dart index f487f880c4..a930181aa0 100644 --- a/modules/angular2/src/transform/common/xhr_impl.dart +++ b/modules/angular2/src/transform/common/xhr_impl.dart @@ -16,9 +16,14 @@ class XhrImpl implements XHR { Future get(String url) async { var assetId = uriToAssetId(_entryPoint, url, logger, null /* span */, errorOnAbsolute: false); + if (assetId == null) { + logger.error( + 'Uri $url not supported from $_entryPoint, could not build AssetId'); + return null; + } var templateExists = await _reader.hasInput(assetId); if (!templateExists) { - logger.error('Could not read template at uri $url from $_entryPoint'); + logger.error('Could not read asset at uri $url from $_entryPoint'); return null; } return await _reader.readAsString(assetId); diff --git a/modules/angular2/src/transform/directive_processor/visitors.dart b/modules/angular2/src/transform/directive_processor/visitors.dart index efbaea4c9c..46ec758ee5 100644 --- a/modules/angular2/src/transform/directive_processor/visitors.dart +++ b/modules/angular2/src/transform/directive_processor/visitors.dart @@ -1,5 +1,6 @@ library angular2.transform.directive_processor.visitors; +import 'dart:async'; import 'package:analyzer/analyzer.dart'; import 'package:analyzer/src/generated/java_core.dart'; import 'package:angular2/src/render/xhr.dart' show XHR; @@ -263,7 +264,7 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { var url = node.expression.accept(_evaluator); if (url is String) { writer.print("template: r'''"); - writer.asyncPrint(_xhr.get(url)); + writer.asyncPrint(_readOrEmptyString(url)); writer.print("'''"); return null; } else { @@ -276,7 +277,7 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { for (var url in urls) { if (url is String) { writer.print("r'''"); - writer.asyncPrint(_xhr.get(url)); + writer.asyncPrint(_readOrEmptyString(url)); writer.print("''', "); } else { logger.warning('style url is not a String ${url}'); @@ -287,4 +288,14 @@ class AnnotationsTransformVisitor extends ToSourceVisitor { } return super.visitNamedExpression(node); } + + /// Attempts to read the content from {@link url}, if it returns null then + /// just return the empty string. + Future _readOrEmptyString(String url) async { + var content = await _xhr.get(url); + if (content == null) { + content = ''; + } + return content; + } } diff --git a/modules/angular2/test/transform/directive_processor/all_tests.dart b/modules/angular2/test/transform/directive_processor/all_tests.dart index 1454467f66..f81f7db4dc 100644 --- a/modules/angular2/test/transform/directive_processor/all_tests.dart +++ b/modules/angular2/test/transform/directive_processor/all_tests.dart @@ -4,6 +4,8 @@ import 'package:barback/barback.dart'; import 'package:angular2/src/transform/directive_processor/rewriter.dart'; import 'package:angular2/src/transform/common/annotation_matcher.dart'; import 'package:angular2/src/transform/common/asset_reader.dart'; +import 'package:angular2/src/transform/common/logging.dart' as log; +import 'package:code_transformers/messages/build_logger.dart'; import 'package:dart_style/dart_style.dart'; import 'package:guinness/guinness.dart'; import 'package:path/path.dart' as path; @@ -67,12 +69,29 @@ void allTests() { _testNgDeps('should not include superclasses in `interfaces`.', 'superclass_files/soup.dart'); + + _testNgDeps( + 'should not throw/hang on invalid urls', 'invalid_url_files/hello.dart', + expectedLogs: [ + 'ERROR: Uri /bad/absolute/url.html not supported from angular2|test/' + 'transform/directive_processor/invalid_url_files/hello.dart, could not ' + 'build AssetId', + 'ERROR: Could not read asset at uri package:invalid/package.css from ' + 'angular2|test/transform/directive_processor/invalid_url_files/' + 'hello.dart', + 'ERROR: Could not read asset at uri bad_relative_url.css from angular2|' + 'test/transform/directive_processor/invalid_url_files/hello.dart' + ]); } void _testNgDeps(String name, String inputPath, {List customDescriptors: const [], AssetId assetId, - AssetReader reader}) { + AssetReader reader, List expectedLogs}) { it(name, () async { + if (expectedLogs != null) { + log.setLogger(new RecordingLogger()); + } + var inputId = _assetIdForPath(inputPath); if (reader == null) { reader = new TestAssetReader(); @@ -93,8 +112,35 @@ void _testNgDeps(String name, String inputPath, expect(formatter.format(output)) .toEqual((await reader.readAsString(expectedId)).replaceAll('\r\n', '\n')); } + + if (expectedLogs != null) { + expect((log.logger as RecordingLogger).logs, expectedLogs); + } }); } AssetId _assetIdForPath(String path) => new AssetId('angular2', 'test/transform/directive_processor/$path'); + +class RecordingLogger implements BuildLogger { + @override + final String detailsUri = ''; + @override + final bool convertErrorsToWarnings = false; + + List logs = []; + + void _record(prefix, msg) => logs.add('$prefix: $msg'); + + void info(msg, {AssetId asset, SourceSpan span}) => _record('INFO', msg); + + void fine(msg, {AssetId asset, SourceSpan span}) => _record('FINE', msg); + + void warning(msg, {AssetId asset, SourceSpan span}) => _record('WARN', msg); + + void error(msg, {AssetId asset, SourceSpan span}) => _record('ERROR', msg); + + Future writeOutput() => throw new UnimplementedError(); + Future addLogFilesFromAsset(AssetId id, [int nextNumber = 1]) => + throw new UnimplementedError(); +} diff --git a/modules/angular2/test/transform/directive_processor/invalid_url_files/expected/hello.ng_deps.dart b/modules/angular2/test/transform/directive_processor/invalid_url_files/expected/hello.ng_deps.dart new file mode 100644 index 0000000000..fd43b3db00 --- /dev/null +++ b/modules/angular2/test/transform/directive_processor/invalid_url_files/expected/hello.ng_deps.dart @@ -0,0 +1,20 @@ +library test.transform.directive_processor.url_expression_files.hello.ng_deps.dart; + +import 'hello.dart'; +import 'package:angular2/angular2.dart' + show bootstrap, Component, Directive, View, NgElement; + +var _visited = false; +void initReflector(reflector) { + if (_visited) return; + _visited = true; + reflector + ..registerType(HelloCmp, { + 'factory': () => new HelloCmp(), + 'parameters': const [], + 'annotations': const [ + const Component(selector: 'hello-app'), + const View(template: r'''''', styles: const [r'''''', r'''''',]) + ] + }); +} diff --git a/modules/angular2/test/transform/directive_processor/invalid_url_files/hello.dart b/modules/angular2/test/transform/directive_processor/invalid_url_files/hello.dart new file mode 100644 index 0000000000..3a0b965ef3 --- /dev/null +++ b/modules/angular2/test/transform/directive_processor/invalid_url_files/hello.dart @@ -0,0 +1,13 @@ +library test.transform.directive_processor.url_expression_files.hello; + +import 'package:angular2/angular2.dart' +show bootstrap, Component, Directive, View, NgElement; + +@Component(selector: 'hello-app') +@View( + templateUrl: '/bad/absolute/url.html', + styleUrls: const [ + 'package:invalid/package.css', + 'bad_relative_url.css' + ]) +class HelloCmp {}