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.

Sign up for GitHub

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

chore: refactor UrlMatcher #1720

Merged
merged 4 commits into from
Dec 28, 2024
Merged

chore: refactor UrlMatcher #1720

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb252dc
chore: refactor UrlMatcher
mxschmitt Dec 19, 2024
97def24
fix NPE
mxschmitt Dec 19, 2024
06b1336
nit
mxschmitt Dec 19, 2024
16c60f6
apply suggestion
mxschmitt Dec 27, 2024
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
80 changes: 51 additions & 29 deletions playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@

import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.function.Predicate;
import java.util.regex.Pattern;
Expand All @@ -30,20 +32,14 @@
import static com.microsoft.playwright.impl.Utils.toJsRegexFlags;

class UrlMatcher {
final Object rawSource;
private final Predicate<String> predicate;

private static Predicate<String> toPredicate(Pattern pattern) {
return s -> pattern.matcher(s).find();
}

static UrlMatcher any() {
return new UrlMatcher((Object) null, null);
}
private final URL baseURL;
Copy link
Member

Choose a reason for hiding this comment

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

Why not resolve the pattern in the constructor and avoid plumbing base url all the way through?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need this later on when I'll add this check:

https://github.com/microsoft/playwright/blob/edd789780ac473083f6fb0b8740faddc43b439ab/packages/playwright-core/src/utils/isomorphic/urlMatch.ts#L101-L103

public final String glob;
public final Pattern pattern;
public final Predicate<String> predicate;

static UrlMatcher forOneOf(URL baseUrl, Object object) {
if (object == null) {
return UrlMatcher.any();
return new UrlMatcher(null, null, null, null);
}
if (object instanceof String) {
return new UrlMatcher(baseUrl, (String) object);
Expand All @@ -68,50 +64,76 @@ static String resolveUrl(URL baseUrl, String spec) {
}
}

UrlMatcher(URL base, String url) {
this(url, toPredicate(Pattern.compile(globToRegex(resolveUrl(base, url)))).or(s -> url == null || url.equals(s)));
UrlMatcher(URL baseURL, String glob) {
this(baseURL, glob, null, null);
}

UrlMatcher(Pattern pattern) {
this(pattern, toPredicate(pattern));
this(null, null, pattern, null);
}

UrlMatcher(Predicate<String> predicate) {
this(predicate, predicate);
this(null, null, null, predicate);
}

private UrlMatcher(Object rawSource, Predicate<String> predicate) {
this.rawSource = rawSource;
private UrlMatcher(URL baseURL, String glob, Pattern pattern, Predicate<String> predicate) {
this.baseURL = baseURL;
this.glob = glob;
this.pattern = pattern;
this.predicate = predicate;
}

boolean test(String value) {
return predicate == null || predicate.test(value);
return testImpl(baseURL, pattern, predicate, glob, value);
}

private static boolean testImpl(URL baseURL, Pattern pattern, Predicate<String> predicate, String glob, String value) {
yury-s marked this conversation as resolved.
Show resolved Hide resolved
if (pattern != null) {
return pattern.matcher(value).find();
}
if (predicate != null) {
return predicate.test(value);
}
if (glob != null) {
return Pattern.compile(globToRegex(resolveUrl(baseURL, glob))).matcher(value).find();
}
return true;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
UrlMatcher that = (UrlMatcher) o;
if (rawSource instanceof Pattern && that.rawSource instanceof Pattern) {
Pattern a = (Pattern) rawSource;
Pattern b = (Pattern) that.rawSource;
return a.pattern().equals(b.pattern()) && a.flags() == b.flags();
if (pattern != null && !pattern.pattern().equals(that.pattern.pattern()) && pattern.flags() == that.pattern.flags()) {
mxschmitt marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
if (predicate != null && !predicate.equals(that.predicate)) {
return false;
}
return Objects.equals(rawSource, that.rawSource);
if (glob != null && !glob.equals(that.glob)) {
return false;
}
return true;
Copy link
Member

Choose a reason for hiding this comment

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

I believe it should be return that.pattern == null && that.predicate == null && that.glob == null;, otherwise it will be equal to any other pattern.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I already have it like that.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree, with the current code e.g.new UrlMatcher(null, null, null, null).equals(new UrlMatcher((String s) -> true) == true which is incorrect.

}

@Override
public int hashCode() {
return Objects.hash(rawSource);
if (pattern != null) {
return pattern.hashCode();
}
if (predicate != null) {
return predicate.hashCode();
}
return glob.hashCode();
mxschmitt marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public String toString() {
if (rawSource == null)
return "<any>";
if (rawSource instanceof Predicate)
return "matching predicate";
return rawSource.toString();
if (pattern != null)
return String.format("<regex pattern=\"%s\" flags=\"%s\">", pattern.pattern(), toJsRegexFlags(pattern));
if (this.predicate != null)
return "<predicate>";
return String.format("<glob pattern=\"%s\">", glob);
}
}
12 changes: 5 additions & 7 deletions playwright/src/main/java/com/microsoft/playwright/impl/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,13 +462,11 @@ static JsonObject interceptionPatterns(List<UrlMatcher> matchers) {
JsonArray jsonPatterns = new JsonArray();
for (UrlMatcher matcher: matchers) {
JsonObject jsonPattern = new JsonObject();
Object urlFilter = matcher.rawSource;
if (urlFilter instanceof String) {
jsonPattern.addProperty("glob", (String) urlFilter);
} else if (urlFilter instanceof Pattern) {
Pattern pattern = (Pattern) urlFilter;
jsonPattern.addProperty("regexSource", pattern.pattern());
jsonPattern.addProperty("regexFlags", toJsRegexFlags(pattern));
if (matcher.glob != null) {
jsonPattern.addProperty("glob", matcher.glob);
} else if (matcher.pattern != null) {
jsonPattern.addProperty("regexSource", matcher.pattern.pattern());
jsonPattern.addProperty("regexFlags", toJsRegexFlags(matcher.pattern));
} else {
// Match all requests.
jsonPattern.addProperty("glob", "**/*");
Expand Down
Loading