Skip to content

Commit 4282003

Browse files
committed
Suppress "Compile called before Add" in re2.Filter
When compiling an empty set, ``FilteredRE2::Compile`` logs a warning to stderr which can not be suppressed (google/re2#485). Replace `re2.Filter` by a null object if the corresponding matchers list is empty: not only do we need to skip `Filter.Compile` to suppress the warning message, we need to skip `Filter.Match` or the program will segfault (google/re2#484). Using a null object seems safer and more reliable than adding conditionals, even if it requires more code and reindenting half the file. Doing this also seems safer than my first instinct of trying to use low-level fd redirection: fd redirection suffers from race conditions[^thread] and could suffer from other cross-platform compatibility issues (e.g. does every python-supported OS have stderr on fd 2 and correctly supports dup, dup2, and close?) [^thread]: AFAIK CPython does not provide a python-level GIL-pin feature (even less so with the GILectomy plans), so we have no way to prevent context-switching and any message sent to stderr by sibling threads would be lost
1 parent b45380d commit 4282003

3 files changed

Lines changed: 47 additions & 19 deletions

File tree

pyproject.toml

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,8 @@ ignore = [
5151
]
5252

5353
[tool.ruff.lint.isort]
54-
classes = ["LRU", "OS"]
54+
classes = ["OS"]
55+
known-first-party = ["ua_parser"]
5556
combine-as-imports = true
5657

5758
[tool.mypy]

src/ua_parser/re2.py

Lines changed: 32 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,11 @@
1616
)
1717

1818

19+
class DummyFilter:
20+
def Match(self, _: str) -> None:
21+
pass
22+
23+
1924
class Resolver:
2025
ua: re2.Filter
2126
user_agent_matchers: List[Matcher[UserAgent]]
@@ -30,26 +35,35 @@ def __init__(
3035
) -> None:
3136
self.user_agent_matchers, self.os_matchers, self.device_matchers = matchers
3237

33-
self.ua = re2.Filter()
34-
for u in self.user_agent_matchers:
35-
self.ua.Add(u.pattern)
36-
self.ua.Compile()
38+
if self.user_agent_matchers:
39+
self.ua = re2.Filter()
40+
for u in self.user_agent_matchers:
41+
self.ua.Add(u.pattern)
42+
self.ua.Compile()
43+
else:
44+
self.ua = DummyFilter()
3745

38-
self.os = re2.Filter()
39-
for o in self.os_matchers:
40-
self.os.Add(o.pattern)
41-
self.os.Compile()
46+
if self.os_matchers:
47+
self.os = re2.Filter()
48+
for o in self.os_matchers:
49+
self.os.Add(o.pattern)
50+
self.os.Compile()
51+
else:
52+
self.os = DummyFilter()
4253

43-
self.devices = re2.Filter()
44-
for d in self.device_matchers:
45-
# Prepend the i global flag if IGNORECASE is set. Assumes
46-
# no pattern uses global flags, but since they're not
47-
# supported in JS that seems safe.
48-
if d.flags & re.IGNORECASE:
49-
self.devices.Add("(?i)" + d.pattern)
50-
else:
51-
self.devices.Add(d.pattern)
52-
self.devices.Compile()
54+
if self.device_matchers:
55+
self.devices = re2.Filter()
56+
for d in self.device_matchers:
57+
# Prepend the i global flag if IGNORECASE is set. Assumes
58+
# no pattern uses global flags, but since they're not
59+
# supported in JS that seems safe.
60+
if d.flags & re.IGNORECASE:
61+
self.devices.Add("(?i)" + d.pattern)
62+
else:
63+
self.devices.Add(d.pattern)
64+
self.devices.Compile()
65+
else:
66+
self.devices = DummyFilter()
5367

5468
def __call__(self, ua: str, domains: Domain, /) -> PartialParseResult:
5569
user_agent = os = device = None

tests/test_re2.py

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
import pytest # type: ignore
2+
3+
from ua_parser import Domain, PartialParseResult
4+
5+
re2 = pytest.importorskip("ua_parser.re2")
6+
7+
8+
def test_empty(capfd: pytest.CaptureFixture[str]) -> None:
9+
r = re2.Resolver(([], [], []))
10+
assert r("", Domain.ALL) == PartialParseResult(Domain.ALL, None, None, None, "")
11+
out, err = capfd.readouterr()
12+
assert out == ""
13+
assert err == ""

0 commit comments

Comments
 (0)