Make error handling optionally more permissive. #58

lrhn · 2020-02-06T13:29:38Z

Fixes a bug.

jensjoha · 2020-02-06T13:38:53Z

lib/src/package_config_json.dart

+    onError(PackageConfigFormatException(e.message, e.source, e.offset));
+    return null;
+  } on Object catch (e) {
+    print("WTF: $e");


Maybe not...

Debug-print, FTW!

…t be read.

jensjoha · 2020-02-06T14:04:26Z

For the following packages file:

foo:lib/#dart=2.6

I get 3 errors:

org-dartlang-test:///a/b/c/.packages: Error: Problem in packages configuration file: Location URI must not have query or fragment
org-dartlang-test:///a/b/c/.packages: Error: Problem in packages configuration file: Not an absolute URI with no query or fragment with a path ending in /
org-dartlang-test:///a/b/c/.packages: Error: Problem in packages configuration file: Not an absolute URI with no query or fragment with a path ending in /

Also - even though it's a FormatException with support for a location, there is none...

jensjoha · 2020-02-06T14:09:15Z

lib/src/package_config_impl.dart

+        checkValidVersionNumber(languageVersion) >= 0) {
+      onError(PackageConfigArgumentError(languageVersion, "languageVersion",
+          "Invalid language version format"));
+      languageVersion = null;


Could we get a special marker for invalid language version?

What kind of "special marker"?

So a client can tell that a language version was set, although it was invalid. It my suggested hack-patch I simply overwrite the string "invalid" instead of null --- though something more elegant is surely possible...

jensjoha · 2020-02-06T14:12:15Z

{
    "configVersion": 2,
    "packages": [
        {
            "name": "foo",
            "rootUri": "../lib/",
            "languageVersion": "arglebargle",
            "languageVersion": "2.5"
        }
    ]
}

just gets language version 2.5.

jensjoha · 2020-02-06T14:39:37Z

While this might not be pretty, this fixes the most severe issues I have:

diff --git a/lib/src/package_config_impl.dart b/lib/src/package_config_impl.dart
index fb52a23..676854f 100644
--- a/lib/src/package_config_impl.dart
+++ b/lib/src/package_config_impl.dart
@@ -223,7 +223,7 @@ class SimplePackage implements Package {
         checkValidVersionNumber(languageVersion) >= 0) {
       onError(PackageConfigArgumentError(languageVersion, "languageVersion",
           "Invalid language version format"));
-      languageVersion = null;
+      languageVersion = "invalid";
     }
     if (fatalError) return null;
     return SimplePackage._(
diff --git a/lib/src/package_config_json.dart b/lib/src/package_config_json.dart
index 6528526..d3dd69e 100644
--- a/lib/src/package_config_json.dart
+++ b/lib/src/package_config_json.dart
@@ -238,7 +238,8 @@ PackageConfig parsePackageConfigJson(
           packageUri = checkType<String>(value, _packageUriKey, name);
           break;
         case _languageVersionKey:
-          languageVersion = checkType<String>(value, _languageVersionKey, name);
+          languageVersion =
+              checkType<String>(value, _languageVersionKey, name) ?? "invalid";
           break;
         default:
           (extraData ??= {})[key] = value;
diff --git a/lib/src/packages_file.dart b/lib/src/packages_file.dart
index 1bf3bf6..7e547ff 100644
--- a/lib/src/packages_file.dart
+++ b/lib/src/packages_file.dart
@@ -86,6 +86,12 @@ PackageConfig parse(
     if (packageLocation.hasQuery || packageLocation.hasFragment) {
       onError(PackageConfigFormatException(
           "Location URI must not have query or fragment", source, start));
+      packageLocation = new Uri(
+          scheme: packageLocation.scheme,
+          userInfo: packageLocation.userInfo,
+          host: packageLocation.host,
+          port: packageLocation.port,
+          path: packageLocation.path);
     }
     if (!packageLocation.path.endsWith('/')) {
       packageLocation =

It does not add offsets, but that's only a minor issue I think.
It also doesn't fix the json parsing taking the most reason value for a key, but offline you talked about that getting fixes once you do a more performant version of this package, so I can probably wait for that...

lrhn · 2020-02-07T09:28:23Z

For the following packages file:

foo:lib/#dart=2.6

I get 3 errors:

org-dartlang-test:///a/b/c/.packages: Error: Problem in packages configuration file: Location URI must not have query or fragment
org-dartlang-test:///a/b/c/.packages: Error: Problem in packages configuration file: Not an absolute URI with no query or fragment with a path ending in /
org-dartlang-test:///a/b/c/.packages: Error: Problem in packages configuration file: Not an absolute URI with no query or fragment with a path ending in /

Also - even though it's a FormatException with support for a location, there is none...

That's a lot of errors.
It's explainable ... while I report the error in the parser, I don't correct it, so it later tries to use the invalid path, it's detected again (in the SimplePackage factory).

I'll try fixing the issue instead.

Make more errors be format exceptions with offset. Don't continue with an invalid value. Instead either sanitize the value, if possible, ignore it if it was optional, or bail out if it is required.

Allow invalid language versions to be detectable.

jensjoha · 2020-02-07T12:22:34Z

CHANGELOG.md

+- Do not require root URIs to have paths starting with `/`. That
+  only makes sense for `file` or `http`, and they enforce it anyway.
+- Fixed bug in language version validation not accepting the digit `9`.
+


Missing entry for language version becoming an object rather than a string.

jensjoha · 2020-02-07T12:27:58Z

lib/src/package_config_json.dart

-      throw PackageConfigFormatException(e.message, e.invalidValue);
+
+    LanguageVersion /*?*/ version;
+    if (languageVersion != null) {


languageVersion is null if it's not a string in the json format (line 240 --- for some reason I cannot mark that line).

jensjoha · 2020-02-07T12:31:43Z

pubspec.yaml

@@ -1,15 +1,11 @@
-name: package_config
-version: 2.0.0
+name: package_config_2


This would require tests to be updated too, right?

It should match, I forgot to change this one back after running tests.

Don't write invalid language versions.

jensjoha

LGTM.

…nfig#58) * Make error handling optionally more permissive. * Make the LanguageVersion be an object, not a string. * Make it version 3.0.0-dev

Make error handling optionally more permissive.

94db200

Fixes a bug.

lrhn requested a review from jensjoha February 6, 2020 13:29

googlebot added the cla: yes label Feb 6, 2020

jensjoha reviewed Feb 6, 2020

View reviewed changes

Address comments, consistently return an empty config if a file canno…

6e6f82a

…t be read.

jensjoha reviewed Feb 6, 2020

View reviewed changes

lrhn added 2 commits February 7, 2020 11:14

Address comments.

f8074ec

Make more errors be format exceptions with offset. Don't continue with an invalid value. Instead either sanitize the value, if possible, ignore it if it was optional, or bail out if it is required.

Make the LanguageVersion be an object, not a string.

6321f89

Allow invalid language versions to be detectable.

jensjoha reviewed Feb 7, 2020

View reviewed changes

Name

1625f10

jensjoha reviewed Feb 7, 2020

View reviewed changes

lrhn added 3 commits February 7, 2020 13:33

Working

c41602c

Update changelog.

80fce6f

Recognize a non-string language version entry as an invalid version.

bef65bf

Don't write invalid language versions.

lrhn force-pushed the permissive-parsing branch from fb180d4 to d7f133c Compare February 7, 2020 12:49

Don't be verbose in eror marker.

cbd07f9

lrhn force-pushed the permissive-parsing branch from d7f133c to cbd07f9 Compare February 7, 2020 12:50

jensjoha approved these changes Feb 7, 2020

View reviewed changes

lrhn added 2 commits February 7, 2020 14:36

Don't make it an error to not have slash after root URI.

8e776a8

Make it version 3.0.0-dev

13f94f7

lrhn merged commit 96ea916 into master Feb 11, 2020

kevmoo deleted the permissive-parsing branch February 21, 2020 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make error handling optionally more permissive. #58

Make error handling optionally more permissive. #58

lrhn commented Feb 6, 2020

jensjoha Feb 6, 2020

lrhn Feb 6, 2020

jensjoha commented Feb 6, 2020

jensjoha Feb 6, 2020

lrhn Feb 7, 2020

jensjoha Feb 7, 2020

jensjoha commented Feb 6, 2020

jensjoha commented Feb 6, 2020

lrhn commented Feb 7, 2020

jensjoha Feb 7, 2020

jensjoha Feb 7, 2020

jensjoha Feb 7, 2020

lrhn Feb 7, 2020

jensjoha left a comment

Make error handling optionally more permissive. #58

Make error handling optionally more permissive. #58

Conversation

lrhn commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jensjoha commented Feb 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jensjoha commented Feb 6, 2020

jensjoha commented Feb 6, 2020

lrhn commented Feb 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jensjoha left a comment

Choose a reason for hiding this comment