From 2fab6f4fbad72e77a679be5a3cf5e3fa1f45486c Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 11:15:08 -0700 Subject: [PATCH 1/8] Raise an exception when serializing CQMs with bad labels Closes https://github.com/dwavesystems/dimod/issues/1358 --- dimod/constrained/constrained.py | 10 +++++++ ...ls-and-serialization-c0eb5e410293a63c.yaml | 7 +++++ tests/test_constrained.py | 28 +++++++++++++++++++ 3 files changed, 45 insertions(+) create mode 100644 releasenotes/notes/fix-constraint-labels-and-serialization-c0eb5e410293a63c.yaml diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index 86c4fc93c..dbddb7e07 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -1785,6 +1785,16 @@ def to_file(self, *, # put everything in a constraints/label/ directory lstr = json.dumps(serialize_variable(label)) + # We want to disallow invalid filenames. We do this via a blacklist + # rather than a whitelist to be as permissive as possible. If we find enough + # other edge cases, we can switch in the future. + # Also, if we find that raising an error places an undue burden on users, we can + # switch to a scheme where invalid names are saved as files with generic directory + # labels. + if "/" in lstr: + # NULL actually passes fine because of the JSON dumps + raise ValueError("Cannot serialize constraint labels containing '/'") + with zf.open(f'constraints/{lstr}/lhs', "w", force_zip64=True) as fdst: constraint.lhs._into_file(fdst) diff --git a/releasenotes/notes/fix-constraint-labels-and-serialization-c0eb5e410293a63c.yaml b/releasenotes/notes/fix-constraint-labels-and-serialization-c0eb5e410293a63c.yaml new file mode 100644 index 000000000..995b0e1ef --- /dev/null +++ b/releasenotes/notes/fix-constraint-labels-and-serialization-c0eb5e410293a63c.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Raise an exception when serializing constrained quadratic models with constraint + labels containing ``"/"``. Previously they would be serialized but would subsequently + break deserialization. + See `#1358 `_. diff --git a/tests/test_constrained.py b/tests/test_constrained.py index 54941b012..8c9d1398f 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -1334,6 +1334,34 @@ def test_unused_variable(self): self.assertEqual(new.lower_bound(v), cqm.lower_bound(v)) self.assertEqual(new.upper_bound(v), cqm.upper_bound(v)) + def test_unusual_constraint_labels(self): + x, y = dimod.Binaries("xy") + + with self.subTest("/"): + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label="hello/world") + with self.assertRaises(ValueError): + cqm.to_file() + + # NULL actually works because it's passed through JSON + with self.subTest("NULL"): + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label="hello\0world") + with cqm.to_file() as f: + new = dimod.CQM.from_file(f) + + self.assertEqual(list(new.constraints), ["hello\0world"]) + + # a few other potentially tricky characters, not exhaustive + for c in ";,\\>|😜+-&": + with self.subTest(c): + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=f"hello{c}world") + with cqm.to_file() as f: + new = dimod.CQM.from_file(f) + + self.assertEqual(list(new.constraints), [f"hello{c}world"]) + class TestSetObjective(unittest.TestCase): def test_bqm(self): From f08e36eca1f41567e330a533b17036bacbf5555e Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 11:36:25 -0700 Subject: [PATCH 2/8] Add a debug statement for windows --- .circleci/config.yml | 100 +++++++++++++++---------------- dimod/constrained/constrained.py | 7 ++- 2 files changed, 55 insertions(+), 52 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 1d4229b1e..3ec2c1268 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -310,54 +310,54 @@ workflows: matrix: parameters: python-version: &python-versions [3.8.9, 3.9.4, 3.10.0, 3.11.0] - - build-linux-aarch64: *build - - build-sdist - - build-osx: *build + # - build-linux-aarch64: *build + # - build-sdist + # - build-osx: *build - build-windows: *build - - test-airspeed-velocity - - test-codecov - - test-doctest - - test-linux: - name: test-linux-<< matrix.dependencies >>-py<< matrix.python-version >> - requires: - - build-linux - matrix: - parameters: - # test the lowest supported numpy and the latest - dependencies: [oldest-supported-numpy, numpy] - python-version: *python-versions - - test-linux-cpp - - test-osx-cpp - - test-sdist: - requires: - - build-sdist - deploy: - jobs: - - build-linux: &deploy-build - <<: *build - filters: - tags: - only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ - branches: - ignore: /.*/ - - build-linux-aarch64: *deploy-build - - build-osx: *deploy-build - - build-sdist: - filters: - tags: - only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ - branches: - ignore: /.*/ - - build-windows: *deploy-build - - deploy-all: - filters: - tags: - only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ - branches: - ignore: /.*/ - requires: - - build-linux - - build-linux-aarch64 - - build-osx - - build-sdist - - build-windows + # - test-airspeed-velocity + # - test-codecov + # - test-doctest + # - test-linux: + # name: test-linux-<< matrix.dependencies >>-py<< matrix.python-version >> + # requires: + # - build-linux + # matrix: + # parameters: + # # test the lowest supported numpy and the latest + # dependencies: [oldest-supported-numpy, numpy] + # python-version: *python-versions + # - test-linux-cpp + # - test-osx-cpp + # - test-sdist: + # requires: + # - build-sdist + # deploy: + # jobs: + # - build-linux: &deploy-build + # <<: *build + # filters: + # tags: + # only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ + # branches: + # ignore: /.*/ + # - build-linux-aarch64: *deploy-build + # - build-osx: *deploy-build + # - build-sdist: + # filters: + # tags: + # only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ + # branches: + # ignore: /.*/ + # - build-windows: *deploy-build + # - deploy-all: + # filters: + # tags: + # only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ + # branches: + # ignore: /.*/ + # requires: + # - build-linux + # - build-linux-aarch64 + # - build-osx + # - build-sdist + # - build-windows diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index dbddb7e07..9d9ffaa25 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -1074,8 +1074,11 @@ def from_file(cls, if match is not None: constraint_labels.add(match.group(1)) - for constraint in constraint_labels: - label = deserialize_variable(json.loads(constraint)) + for constraint in constraint_labels: + try: + label = deserialize_variable(json.loads(constraint)) + except json.decoder.JSONDecodeError: + raise RuntimeError(f"Cannot load {constraint!r}, namelist={zf.namelist()}") rhs = np.frombuffer(zf.read(f"constraints/{constraint}/rhs"), np.float64)[0] sense = zf.read(f"constraints/{constraint}/sense").decode('ascii') From c00eecfd07dee1f0d56a2db4f2a2912e5e3de115 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 12:03:12 -0700 Subject: [PATCH 3/8] Try a more general fix --- dimod/constrained/constrained.py | 15 +++------------ tests/test_constrained.py | 6 ++++-- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index 9d9ffaa25..b830a6975 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -961,7 +961,8 @@ def _from_file_legacy(cls, constraint_labels = set() for arch in zf.namelist(): # even on windows zip uses / - match = re.match("constraints/([^/]+)/", arch) + # rely on the fact that we have at least an lhs file + match = re.match("^constraints/(.+)/lhs$", arch) if match is not None: constraint_labels.add(match.group(1)) @@ -1070,7 +1071,7 @@ def from_file(cls, constraint_labels = set() for arch in zf.namelist(): # even on windows zip uses / - match = re.match("constraints/([^/]+)/", arch) + match = re.match("^constraints/(.+)/lhs$", arch) if match is not None: constraint_labels.add(match.group(1)) @@ -1788,16 +1789,6 @@ def to_file(self, *, # put everything in a constraints/label/ directory lstr = json.dumps(serialize_variable(label)) - # We want to disallow invalid filenames. We do this via a blacklist - # rather than a whitelist to be as permissive as possible. If we find enough - # other edge cases, we can switch in the future. - # Also, if we find that raising an error places an undue burden on users, we can - # switch to a scheme where invalid names are saved as files with generic directory - # labels. - if "/" in lstr: - # NULL actually passes fine because of the JSON dumps - raise ValueError("Cannot serialize constraint labels containing '/'") - with zf.open(f'constraints/{lstr}/lhs', "w", force_zip64=True) as fdst: constraint.lhs._into_file(fdst) diff --git a/tests/test_constrained.py b/tests/test_constrained.py index 8c9d1398f..0534c0669 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -1340,8 +1340,10 @@ def test_unusual_constraint_labels(self): with self.subTest("/"): cqm = dimod.CQM() cqm.add_constraint(x + y <= 5, label="hello/world") - with self.assertRaises(ValueError): - cqm.to_file() + with cqm.to_file() as f: + new = dimod.CQM.from_file(f) + + self.assertEqual(list(new.constraints), ["hello/world"]) # NULL actually works because it's passed through JSON with self.subTest("NULL"): From 0fd43c1a52821f6ab6623fa6d14865aeb37ecca8 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 12:36:20 -0700 Subject: [PATCH 4/8] And again --- dimod/constrained/constrained.py | 6 +++ tests/test_constrained.py | 70 ++++++++++++++++++++++++-------- 2 files changed, 58 insertions(+), 18 deletions(-) diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index b830a6975..920a4ac35 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -1789,6 +1789,12 @@ def to_file(self, *, # put everything in a constraints/label/ directory lstr = json.dumps(serialize_variable(label)) + if os.sep != "/" and os.sep in lstr: + # Irritatingly, zipfile will automatically swap \ to / in windows + # with no way to prevent it. So we need to disallow them otherwise we don't get + # symmetric deserialization. + raise ValueError("cannot serialize constraint labels with '\\' on windows") + with zf.open(f'constraints/{lstr}/lhs', "w", force_zip64=True) as fdst: constraint.lhs._into_file(fdst) diff --git a/tests/test_constrained.py b/tests/test_constrained.py index 0534c0669..fcf726059 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -1335,34 +1335,68 @@ def test_unused_variable(self): self.assertEqual(new.upper_bound(v), cqm.upper_bound(v)) def test_unusual_constraint_labels(self): + import os + x, y = dimod.Binaries("xy") - with self.subTest("/"): - cqm = dimod.CQM() - cqm.add_constraint(x + y <= 5, label="hello/world") - with cqm.to_file() as f: - new = dimod.CQM.from_file(f) + unusual_characters = "/\0" # typically disallowed in filenames + unusual_characters += " .~;,>|😜+-&" # other tricky ones, no exhaustive - self.assertEqual(list(new.constraints), ["hello/world"]) + if os.sep == "/": + unusual_characters += "/" + else: + # in windows we disallow serializing with "/" + with self.subTest("windows /"): + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label="test/") + with self.assertRaises(ValueError): + cqm.to_file() - # NULL actually works because it's passed through JSON - with self.subTest("NULL"): - cqm = dimod.CQM() - cqm.add_constraint(x + y <= 5, label="hello\0world") - with cqm.to_file() as f: - new = dimod.CQM.from_file(f) + for char in unusual_characters: + with self.subTest(f"leading {char}"): + label = f"{char}test" + + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=label) + with cqm.to_file() as f: + new = dimod.CQM.from_file(f) + + # best we can hope for is an equivalent after a json round trip + self.assertEqual(list(new.constraints), [json.loads(json.dumps(label))]) - self.assertEqual(list(new.constraints), ["hello\0world"]) + for char in unusual_characters: + with self.subTest(f"trailing {char}"): + label = f"test{char}" - # a few other potentially tricky characters, not exhaustive - for c in ";,\\>|😜+-&": - with self.subTest(c): cqm = dimod.CQM() - cqm.add_constraint(x + y <= 5, label=f"hello{c}world") + cqm.add_constraint(x + y <= 5, label=label) with cqm.to_file() as f: new = dimod.CQM.from_file(f) - self.assertEqual(list(new.constraints), [f"hello{c}world"]) + # best we can hope for is an equivalent after a json round trip + self.assertEqual(list(new.constraints), [json.loads(json.dumps(label))]) + + for char in unusual_characters: + with self.subTest(f"embedded {char}"): + label = f"te{char}st" + + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=label) + with cqm.to_file() as f: + new = dimod.CQM.from_file(f) + + # best we can hope for is an equivalent after a json round trip + self.assertEqual(list(new.constraints), [json.loads(json.dumps(label))]) + + with self.subTest("empty label"): + label = f"" + + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=label) + with cqm.to_file() as f: + new = dimod.CQM.from_file(f) + + self.assertEqual(list(new.constraints), [label]) class TestSetObjective(unittest.TestCase): From 74eb6dd2f678971d50df06d752a8ef8572b3cc29 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 13:07:42 -0700 Subject: [PATCH 5/8] And again again --- .circleci/config.yml | 100 +++++++++++++++---------------- dimod/constrained/constrained.py | 12 ++-- tests/test_constrained.py | 20 +++---- 3 files changed, 64 insertions(+), 68 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 3ec2c1268..1d4229b1e 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -310,54 +310,54 @@ workflows: matrix: parameters: python-version: &python-versions [3.8.9, 3.9.4, 3.10.0, 3.11.0] - # - build-linux-aarch64: *build - # - build-sdist - # - build-osx: *build + - build-linux-aarch64: *build + - build-sdist + - build-osx: *build - build-windows: *build - # - test-airspeed-velocity - # - test-codecov - # - test-doctest - # - test-linux: - # name: test-linux-<< matrix.dependencies >>-py<< matrix.python-version >> - # requires: - # - build-linux - # matrix: - # parameters: - # # test the lowest supported numpy and the latest - # dependencies: [oldest-supported-numpy, numpy] - # python-version: *python-versions - # - test-linux-cpp - # - test-osx-cpp - # - test-sdist: - # requires: - # - build-sdist - # deploy: - # jobs: - # - build-linux: &deploy-build - # <<: *build - # filters: - # tags: - # only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ - # branches: - # ignore: /.*/ - # - build-linux-aarch64: *deploy-build - # - build-osx: *deploy-build - # - build-sdist: - # filters: - # tags: - # only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ - # branches: - # ignore: /.*/ - # - build-windows: *deploy-build - # - deploy-all: - # filters: - # tags: - # only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ - # branches: - # ignore: /.*/ - # requires: - # - build-linux - # - build-linux-aarch64 - # - build-osx - # - build-sdist - # - build-windows + - test-airspeed-velocity + - test-codecov + - test-doctest + - test-linux: + name: test-linux-<< matrix.dependencies >>-py<< matrix.python-version >> + requires: + - build-linux + matrix: + parameters: + # test the lowest supported numpy and the latest + dependencies: [oldest-supported-numpy, numpy] + python-version: *python-versions + - test-linux-cpp + - test-osx-cpp + - test-sdist: + requires: + - build-sdist + deploy: + jobs: + - build-linux: &deploy-build + <<: *build + filters: + tags: + only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ + branches: + ignore: /.*/ + - build-linux-aarch64: *deploy-build + - build-osx: *deploy-build + - build-sdist: + filters: + tags: + only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ + branches: + ignore: /.*/ + - build-windows: *deploy-build + - deploy-all: + filters: + tags: + only: /^[0-9]+(\.[0-9]+)*((\.dev|rc)([0-9]+)?)?$/ + branches: + ignore: /.*/ + requires: + - build-linux + - build-linux-aarch64 + - build-osx + - build-sdist + - build-windows diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index 920a4ac35..a70d5fb1f 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -1787,13 +1787,13 @@ def to_file(self, *, for label, constraint in self.constraints.items(): # put everything in a constraints/label/ directory - lstr = json.dumps(serialize_variable(label)) + lstr = json.dumps(serialize_variable(label), ensure_ascii=False) - if os.sep != "/" and os.sep in lstr: - # Irritatingly, zipfile will automatically swap \ to / in windows - # with no way to prevent it. So we need to disallow them otherwise we don't get - # symmetric deserialization. - raise ValueError("cannot serialize constraint labels with '\\' on windows") + if "/" in lstr: + # Because of the way we do the regex in .from_file(), we actually do + # support these. But it is inconsistent with the description of the file + # format so we do the simpler thing and just disallow it + raise ValueError("cannot serialize constraint labels containing '/'") with zf.open(f'constraints/{lstr}/lhs', "w", force_zip64=True) as fdst: constraint.lhs._into_file(fdst) diff --git a/tests/test_constrained.py b/tests/test_constrained.py index fcf726059..019a4dac1 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -1339,18 +1339,7 @@ def test_unusual_constraint_labels(self): x, y = dimod.Binaries("xy") - unusual_characters = "/\0" # typically disallowed in filenames - unusual_characters += " .~;,>|😜+-&" # other tricky ones, no exhaustive - - if os.sep == "/": - unusual_characters += "/" - else: - # in windows we disallow serializing with "/" - with self.subTest("windows /"): - cqm = dimod.CQM() - cqm.add_constraint(x + y <= 5, label="test/") - with self.assertRaises(ValueError): - cqm.to_file() + unusual_characters = "\0 .~;,>|😜+-&" # not exhaustive for char in unusual_characters: with self.subTest(f"leading {char}"): @@ -1398,6 +1387,13 @@ def test_unusual_constraint_labels(self): self.assertEqual(list(new.constraints), [label]) + with self.subTest("/"): + label = "test/test" + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=label) + with self.assertRaises(ValueError): + cqm.to_file() + class TestSetObjective(unittest.TestCase): def test_bqm(self): From 4437033377811f930749e1aafd61d504e64762fc Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 13:15:37 -0700 Subject: [PATCH 6/8] Add forward slash case --- dimod/constrained/constrained.py | 10 ++++++---- tests/test_constrained.py | 18 +++++++++++++----- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index a70d5fb1f..5c9db9a39 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -1076,10 +1076,7 @@ def from_file(cls, constraint_labels.add(match.group(1)) for constraint in constraint_labels: - try: - label = deserialize_variable(json.loads(constraint)) - except json.decoder.JSONDecodeError: - raise RuntimeError(f"Cannot load {constraint!r}, namelist={zf.namelist()}") + label = deserialize_variable(json.loads(constraint)) rhs = np.frombuffer(zf.read(f"constraints/{constraint}/rhs"), np.float64)[0] sense = zf.read(f"constraints/{constraint}/sense").decode('ascii') @@ -1795,6 +1792,11 @@ def to_file(self, *, # format so we do the simpler thing and just disallow it raise ValueError("cannot serialize constraint labels containing '/'") + if os.sep == '\\' and os.sep in lstr: + # Irritatingly, zipfile will automatically convert \ to / on windows, so we + # also don't allow that + raise ValueError("cannot serialize constraint labels containing '\\' on windows") + with zf.open(f'constraints/{lstr}/lhs', "w", force_zip64=True) as fdst: constraint.lhs._into_file(fdst) diff --git a/tests/test_constrained.py b/tests/test_constrained.py index 019a4dac1..2d1920e3f 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -1341,6 +1341,16 @@ def test_unusual_constraint_labels(self): unusual_characters = "\0 .~;,>|😜+-&" # not exhaustive + if os.sep == "\\": + with self.subTest("\\"): + label = "test\\test" + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=label) + with self.assertRaises(ValueError): + cqm.to_file() + else: + unusual_characters += "\\" + for char in unusual_characters: with self.subTest(f"leading {char}"): label = f"{char}test" @@ -1351,9 +1361,8 @@ def test_unusual_constraint_labels(self): new = dimod.CQM.from_file(f) # best we can hope for is an equivalent after a json round trip - self.assertEqual(list(new.constraints), [json.loads(json.dumps(label))]) + self.assertEqual(list(new.constraints), [label]) - for char in unusual_characters: with self.subTest(f"trailing {char}"): label = f"test{char}" @@ -1363,9 +1372,8 @@ def test_unusual_constraint_labels(self): new = dimod.CQM.from_file(f) # best we can hope for is an equivalent after a json round trip - self.assertEqual(list(new.constraints), [json.loads(json.dumps(label))]) + self.assertEqual(list(new.constraints), [label]) - for char in unusual_characters: with self.subTest(f"embedded {char}"): label = f"te{char}st" @@ -1375,7 +1383,7 @@ def test_unusual_constraint_labels(self): new = dimod.CQM.from_file(f) # best we can hope for is an equivalent after a json round trip - self.assertEqual(list(new.constraints), [json.loads(json.dumps(label))]) + self.assertEqual(list(new.constraints), [label]) with self.subTest("empty label"): label = f"" From 25ec5312a21032c4f2644a9999da2fa5ed26dac4 Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 13:22:02 -0700 Subject: [PATCH 7/8] Disallow NULL on windows --- tests/test_constrained.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/tests/test_constrained.py b/tests/test_constrained.py index 2d1920e3f..273d67d0c 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -1339,7 +1339,7 @@ def test_unusual_constraint_labels(self): x, y = dimod.Binaries("xy") - unusual_characters = "\0 .~;,>|😜+-&" # not exhaustive + unusual_characters = " .~;,>|😜+-&" # not exhaustive if os.sep == "\\": with self.subTest("\\"): @@ -1348,8 +1348,17 @@ def test_unusual_constraint_labels(self): cqm.add_constraint(x + y <= 5, label=label) with self.assertRaises(ValueError): cqm.to_file() + + # ensure_ascii=False still casts NULL to unicode + # thereby causing it to fail on windows + with self.subTest("\0"): + label = "test\0test" + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=label) + with self.assertRaises(ValueError): + cqm.to_file() else: - unusual_characters += "\\" + unusual_characters += "\0\\" for char in unusual_characters: with self.subTest(f"leading {char}"): From c53c2165021004fbc3f753fdda4a44fc304d0cbd Mon Sep 17 00:00:00 2001 From: Alexander Condello Date: Wed, 13 Sep 2023 13:24:17 -0700 Subject: [PATCH 8/8] Always disallow NULL --- dimod/constrained/constrained.py | 5 +++++ tests/test_constrained.py | 18 ++++++++---------- 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/dimod/constrained/constrained.py b/dimod/constrained/constrained.py index 5c9db9a39..e24c720db 100644 --- a/dimod/constrained/constrained.py +++ b/dimod/constrained/constrained.py @@ -1792,6 +1792,11 @@ def to_file(self, *, # format so we do the simpler thing and just disallow it raise ValueError("cannot serialize constraint labels containing '/'") + if "\0" in lstr: + # Similarily, this actually works, but it's weird and confusing to support it + # so we disallow + raise ValueError("cannot serialize constraint labels containing the NULL character") + if os.sep == '\\' and os.sep in lstr: # Irritatingly, zipfile will automatically convert \ to / on windows, so we # also don't allow that diff --git a/tests/test_constrained.py b/tests/test_constrained.py index 273d67d0c..46317260b 100644 --- a/tests/test_constrained.py +++ b/tests/test_constrained.py @@ -1348,17 +1348,8 @@ def test_unusual_constraint_labels(self): cqm.add_constraint(x + y <= 5, label=label) with self.assertRaises(ValueError): cqm.to_file() - - # ensure_ascii=False still casts NULL to unicode - # thereby causing it to fail on windows - with self.subTest("\0"): - label = "test\0test" - cqm = dimod.CQM() - cqm.add_constraint(x + y <= 5, label=label) - with self.assertRaises(ValueError): - cqm.to_file() else: - unusual_characters += "\0\\" + unusual_characters += "\\" for char in unusual_characters: with self.subTest(f"leading {char}"): @@ -1411,6 +1402,13 @@ def test_unusual_constraint_labels(self): with self.assertRaises(ValueError): cqm.to_file() + with self.subTest("NULL"): + label = "test\0test" + cqm = dimod.CQM() + cqm.add_constraint(x + y <= 5, label=label) + with self.assertRaises(ValueError): + cqm.to_file() + class TestSetObjective(unittest.TestCase): def test_bqm(self):