Skip to content

Add net stdlib module with URL support - #1834

Open
stackoverflow wants to merge 34 commits into
apple:mainfrom
stackoverflow:url-module
Open

stackoverflow wants to merge 34 commits into
apple:mainfrom
stackoverflow:url-module

Conversation

@stackoverflow

Copy link
Copy Markdown
Contributor

No description provided.

@stackoverflow stackoverflow changed the title Add Url stdlib module Add net stdlib module Sep 9, 2026
@stackoverflow stackoverflow changed the title Add net stdlib module Add net stdlib module with URL support Sep 9, 2026
@stackoverflow
stackoverflow marked this pull request as ready for review September 16, 2026 15:34

@bioball bioball left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did a first pass!

Comment thread stdlib/net.pkl Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlFactory.java Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlFactory.java Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlFactory.java Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlNodes.java
Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlNodes.java Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/NetNodes.java Outdated
encodeUtf8(codePoint, out);
}
});
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not use the JDK's java.net.URLEncoder and java.net.URLDecoder?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

URLEncoder/URLDecoder implement application/x-www-form-urlencoded (as stated in their javadoc), not RFC 3986.
For example URLEncoder will encode ~ which is in unreserved in 3986. It also encodes to +.
It could replace our PercentEncoder.encode/decodeForm, but this is a trivial implementation once you already have all the machinery.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I'm referring to encodeForm and decodeForm specifically.

According to WHATWG, the characters to encode also includes U+0021, U+0027-U+0027, and U+007E, but this implementation isn't doing that.

But also, URLEncoder/URLDecoder have already been optimized and battle-tested, so I feel like we might as well use it.

Comment thread pkl-core/src/main/java/org/pkl/core/util/url/UrlParser.java

@HT154 HT154 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here's my first pass

Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlParser.java Outdated
Comment thread stdlib/net.pkl Outdated
Comment thread stdlib/net.pkl
///
/// A URL has an authority exactly when it has a [host].
/// This is [userInfo], [host] and [port] joined back together, encoded as in [toString()].
external fixed authority: String?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
external fixed authority: String?
external authority: String?

External properties already behave as if they're fixed. We don't combine these modifiers anywhere else in the stdlib.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The external modifier has no relation to fixed. The only contract for external properties is that they can't have a default body (it comes from Java).

local m = (import("pkl:math")) {
  minInt = 42
}

bogusMinInt = m.minInt
bogusMinInt = 42

The fact we don't put external + fixed together in other places is because most external properties are in a primitive class (which can't be amended), in an external class (which also can't be amended), or in a module (which can be amended, but doesn't change the original module).

Comment thread stdlib/net.pkl Outdated
Comment thread stdlib/net.pkl Outdated
Comment thread stdlib/net.pkl Outdated

@bioball bioball left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did another pass!

Also: I more and more feel that the Url class should have pairs of raw/encoded properties for each component.

Otherwise, what does this mean?

new net.Url { path = "/foo%20bar" }

There's a big difference between the "this is the encoded value" versus "this is the un-encoded value", and the current API is kind of a hybrid between the two.

Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlFactory.java Outdated
Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/PercentEncoder.java Outdated
}
return UrlFactory.create(UrlParser.resolve(base, ref));
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This implementation (and others) will still create an IndirectCallNode even if it doesn't need it (e.g. it's already in extra storage).

I played around with this and I think an elegant solution here is to introduce a specialized truffle node for it; e.g. this:

package org.pkl.core.ast.internal;

import com.oracle.truffle.api.dsl.Cached;
import com.oracle.truffle.api.dsl.Specialization;
import com.oracle.truffle.api.frame.VirtualFrame;
import com.oracle.truffle.api.nodes.IndirectCallNode;
import com.oracle.truffle.api.source.SourceSection;
import org.pkl.core.ast.PklNode;
import org.pkl.core.runtime.VmTyped;
import org.pkl.core.stdlib.net.UrlFactory;
import org.pkl.core.stdlib.net.UrlParser;

public abstract class GetParsedUrlNode extends PklNode {
  protected GetParsedUrlNode(SourceSection sourceSection) {
    super(sourceSection);
  }

  @Specialization(guards = "url.hasExtraStorage()")
  protected UrlParser.Parsed evalCached(VmTyped url) {
    return (UrlParser.Parsed) url.getExtraStorage();
  }

  @Specialization
  protected UrlParser.Parsed eval(VmTyped url, @Cached("create()") IndirectCallNode callNode) {
    return UrlFactory.read(url, callNode);
  }

  public abstract UrlParser.Parsed execute(VirtualFrame frame, VmTyped url);
}

Then this implementation turns into:

  public abstract static class resolve extends ExternalMethod1Node {
    private @Child GetParsedUrlNode getParsedUrlNode = GetParsedUrlNodeGen.create(sourceSection);

    @Specialization
    protected Object evalString(VirtualFrame frame, VmTyped self, String ref) {
      var base = getParsedUrlNode.execute(frame, self);
      return resolve(base, UrlFactory.parseOrThrow(ref, this));
    }

    @Specialization
    protected Object eval(VirtualFrame frame, VmTyped self, VmTyped ref) {
      var base = getParsedUrlNode.execute(frame, self);
      var parsed = getParsedUrlNode.execute(frame, ref);
      return resolve(base, parsed);
    }

    @SuppressWarnings("MethodNameSameAsClassName")
    private Object resolve(Parsed base, Parsed ref) {
      if (base.scheme() == null) {
        CompilerDirectives.transferToInterpreter();
        throw exceptionBuilder()
            .evalError("cannotResolveAgainstRelativeUrl", base.serialize())
            .build();
      }
      return UrlFactory.create(UrlParser.resolve(base, ref));
    }
  }

Comment thread pkl-core/src/main/java/org/pkl/core/stdlib/net/UrlNodes.java
Comment thread pkl-core/src/main/java/org/pkl/core/util/url/UrlParser.java
static String readPath(VmObjectLike url) {
return url.hasExtraStorage()
? ((Parsed) url.getExtraStorage()).path()
: (String) VmUtils.readMember(url, Identifier.PATH);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Needs a call node

assertThatThrownBy(() -> map("ipvFuture"))
.isInstanceOf(ConversionException.class)
.hasMessageContaining("http://[v1.fe80::a+en1]/")
.hasCauseInstanceOf(URISyntaxException.class);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It doesn't seem great that you can define valid data in Pkl and have that blow up when mapped to Java, although it's definitely an edge case.

encodeUtf8(codePoint, out);
}
});
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, I'm referring to encodeForm and decodeForm specifically.

According to WHATWG, the characters to encode also includes U+0021, U+0027-U+0027, and U+007E, but this implementation isn't doing that.

But also, URLEncoder/URLDecoder have already been optimized and battle-tested, so I feel like we might as well use it.

* <p>A {@code %} that does not begin a percent-encoded octet is kept as-is.
*/
static String decode(String input) {
var in = input.getBytes(StandardCharsets.UTF_8);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Optimization: we can skip the allocation overhead if there's no % to begin with.

Suggested change
var in = input.getBytes(StandardCharsets.UTF_8);
if (!input.contains("%")) {
return input;
}
var in = input.getBytes(StandardCharsets.UTF_8);

Comment thread stdlib/net.pkl
/// Percent-decodes [value], interpreting the decoded bytes as UTF-8.
///
/// The inverse of [encodeUrlComponent()].
/// A `%` that does not begin a percent-encoded octet is kept as-is.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I strongly feel that this should throw, not kept as-is. Otherwise, a malformed percent encoding would silently be passed through as a decoded string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants