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

Error when JSON/YAML parsing would set property default in a Dynamic #700

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import org.pkl.core.stdlib.ExternalMethod1Node;
import org.pkl.core.stdlib.PklConverter;
import org.pkl.core.util.EconomicMaps;
import org.pkl.core.util.ErrorMessages;
import org.pkl.core.util.Nullable;
import org.pkl.core.util.json.JsonHandler;
import org.pkl.core.util.json.JsonParser;
Expand Down Expand Up @@ -172,7 +173,13 @@ public void endObject(@Nullable EconomicMap<Object, ObjectMember> members) {

@Override
public void startObjectValue(@Nullable EconomicMap<Object, ObjectMember> members, String name) {
currPath.push(Identifier.get(name));
var identifier = Identifier.get(name);
if (!useMapping && identifier == Identifier.DEFAULT) {
// https://github.com/apple/pkl/issues/561
throw new ParseException(
HT154 marked this conversation as resolved.
Show resolved Hide resolved
ErrorMessages.create("jsonParseErrorDynamicPropertyDefault"), getLocation());
}
currPath.push(identifier);
}

@Override
Expand Down
11 changes: 11 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/stdlib/yaml/ParserNodes.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import org.pkl.core.stdlib.ExternalMethod1Node;
import org.pkl.core.stdlib.PklConverter;
import org.pkl.core.util.EconomicMaps;
import org.pkl.core.util.ErrorMessages;
import org.pkl.core.util.yaml.ParseException;
import org.pkl.core.util.yaml.snake.YamlUtils;
import org.snakeyaml.engine.v2.api.ConstructNode;
import org.snakeyaml.engine.v2.api.Load;
Expand Down Expand Up @@ -115,6 +117,8 @@ private VmList doParseAll(VmTyped self, String text, String uri) {
.build();
}
throw exceptionBuilder().evalError("yamlParseError").withHint(e.getMessage()).build();
} catch (ParseException e) {
throw exceptionBuilder().evalError("yamlParseError").withHint(e.getMessage()).build();
}

return builder.build();
Expand Down Expand Up @@ -469,6 +473,13 @@ private void addMembers(MappingNode node, VmObject object) {
var memberName =
convertedKey instanceof String string && !useMapping ? Identifier.get(string) : null;

// https://github.com/apple/pkl/issues/561
if (memberName == Identifier.DEFAULT) {
throw new ParseException(
Copy link
Contributor

@odenix odenix Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All other error handling code in this file throws YamlEngineException, which is caught in the same file and translated to VmException. Why throw a custom exception here, only to duplicate the code that translates to VmException? (I know this was suggested in the review, but I don't see the point.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

YamlEngineException doesn't feel like the right class to throw our own errors with tailored messages. I think the direction we should move here is to switch all of our own throws from YamlEngineException to org.pkl.core.util.yaml.ParseException.

Copy link
Contributor

@odenix odenix Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwing ParseException everywhere is an improvement. But since these exceptions are implementation details and thrown and caught in the same file, introducing your own exception feels unnecessary (adds code for no benefit).

json.ParserNodes follows the same pattern: It throws org.pkl.core.util.json.ParseException, which also isn't your own exception.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the extra class seems needlessly verbose and that we should probably move away from YamlEngineExceptions. I don't quite understand org.pkl.core.util.json.ParseException not being "your own exception." What do you mean by that, @translatenix?

Is there any reason not to move org.pkl.core.util.json.ParseException up one level (into util) and use it for either?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that errors coming from ParseException can be presented to users as-is; because they are already tailored to be presented as error messages.

YamlEngineException messages come from SnakeYAML, and should be decorated with a generic "malformed yaml" message.

I think a good north star here is to not surface messages from YamlEngineException at all, and instead present all parse errors as our own tailored error messages. But, for now, it's helpful to have two different exception classes so we know what comes from us, and what comes from our parser library.

Copy link
Contributor

@odenix odenix Oct 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand org.pkl.core.util.json.ParseException not being "your own exception." What do you mean by that, @translatenix?

org.pkl.core.util.json.ParseException comes from the external JSON library that was copied into Pkl's codebase. As such it's the equivalent of YamlEngineException .

YamlEngineException messages come from SnakeYAML, and should be decorated with a generic "malformed yaml" message.

If that's what you want, having your own exception makes sense, assuming you can't throw a VmException to begin with. You may want to make the same change for json/ParserNodes.

ErrorMessages.create("yamlParseErrorDynamicPropertyDefault"),
keyNode.getStartMark().isEmpty() ? null : keyNode.getStartMark().get());
}

var member =
new ObjectMember(
sourceSection,
Expand Down
38 changes: 38 additions & 0 deletions pkl-core/src/main/java/org/pkl/core/util/yaml/ParseException.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
/*
* Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* https://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.pkl.core.util.yaml;

import org.pkl.core.util.Nullable;
import org.snakeyaml.engine.v2.exceptions.Mark;

/** An unchecked exception to indicate that an input does not qualify as valid YAML. */
public final class ParseException extends RuntimeException {
private final @Nullable Mark location;

public ParseException(String message, @Nullable Mark location) {
super(location == null ? message : message + location);
this.location = location;
}

/**
* Returns the location at which the error occurred.
*
* @return the error location
*/
public @Nullable Mark getLocation() {
return location;
}
}
10 changes: 10 additions & 0 deletions pkl-core/src/main/resources/org/pkl/core/errorMessages.properties
Original file line number Diff line number Diff line change
Expand Up @@ -796,6 +796,11 @@ Converter path `{0}` has invalid syntax.
jsonParseError=\
Error parsing JSON document.

jsonParseErrorDynamicPropertyDefault=\
Cannot parse an object with key `"default"` into a `Dynamic`.\
\n\
Try parsing into a `Mapping` instead, by setting `useMapping = true` in the `pkl.json#Parser`.

yamlParseError=\
Error parsing YAML document.

Expand All @@ -804,6 +809,11 @@ Error parsing YAML document: The number of aliases for collection nodes exceeds
\n\
To increase the allowed maximum, set `YamlRenderer.maxCollectionAliases`.

yamlParseErrorDynamicPropertyDefault=\
Cannot parse an object with key `default` into a `Dynamic`.\
\n\
Try parsing into a `Mapping` instead, by setting `useMapping = true` in the `pkl.yaml#Parser`.

evaluationTimedOut=\
Evaluation timed out after {0,number,#.##} second(s).

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,3 +66,11 @@ res16 = parser.parse("""

// invalid syntax
res17 = test.catch(() -> parser.parse("!@#$%"))

res18 = test.catch(() -> parser.parse("""
{"default": null}
"""))

res19 = (parser) { useMapping = true }.parse("""
{"default": null}
""")
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,11 @@ res15 = parser.parseAll("""
hobby: surfing
...
""")

res16 = test.catch(() -> parser.parse("""
{"default": null}
"""))

res17 = (parser) { useMapping = true }.parse("""
default: null
""")
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,7 @@ res16 {
}
}
res17 = "Error parsing JSON document."
res18 = "Error parsing JSON document."
res19 {
["default"] = null
}
Original file line number Diff line number Diff line change
Expand Up @@ -47,3 +47,7 @@ res15 = List(new {
}, new {
hobby = "surfing"
})
res16 = "Error parsing YAML document."
res17 {
["default"] = null
}