Skip to content

Commit 581dc1d

Browse files
committed
Address review: reject blank label text, drop empty strings before create
- Reject text composed of whitespace early with a stable error message, matching the reviewer's request that text not silently fall back to the Text Label model default - Tighten create_kwargs filter to drop empty strings as well as None, preventing blank values from bypassing model defaults at the DB level - Add regression test for the new validation path
1 parent 3e5eb62 commit 581dc1d

2 files changed

Lines changed: 34 additions & 3 deletions

File tree

config/graphql/label_mutations.py

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,19 @@ def mutate(
237237
obj = None
238238
obj_id = None
239239

240+
# Reject blank ``text`` early. ``text`` is GraphQL-optional so the
241+
# model default ("Text Label") would silently apply, which produces
242+
# surprising labels for clients that forgot to pass a value or
243+
# passed an empty string. Django's ``blank=False`` is form-only
244+
# and is NOT enforced by ``objects.create()``.
245+
if text is not None and not text.strip():
246+
return CreateLabelForLabelsetMutation(
247+
obj=None,
248+
obj_id=None,
249+
message="Label text cannot be blank.",
250+
ok=False,
251+
)
252+
240253
# Validate color format (defense in depth)
241254
is_valid_color, color_error = validate_color(color)
242255
if not is_valid_color:
@@ -255,8 +268,10 @@ def mutate(
255268
# Generic deny path — same message and code path as not-found
256269
raise LabelSet.DoesNotExist()
257270
logger.debug("CreateLabelForLabelsetMutation - mutate / Labelset", labelset)
258-
# Drop None values so model field defaults apply (description,
259-
# color, icon, text are NOT NULL with sensible defaults).
271+
# Drop None and empty-string values so model field defaults
272+
# apply (description, color, icon, text are NOT NULL with
273+
# sensible defaults). Empty strings would bypass those
274+
# defaults at the DB level and write a row with blank values.
260275
create_kwargs = {
261276
k: v
262277
for k, v in {
@@ -266,7 +281,7 @@ def mutate(
266281
"icon": icon,
267282
"label_type": label_type,
268283
}.items()
269-
if v is not None
284+
if v is not None and v != ""
270285
}
271286
obj = AnnotationLabel.objects.create(
272287
creator=info.context.user, **create_kwargs

opencontractserver/tests/test_label_mutations.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -362,3 +362,19 @@ def test_non_owner_with_explicit_update_permission_can_create(self) -> None:
362362
new_label = self.labelset.annotation_labels.first()
363363
self.assertEqual(new_label.text, "Collaborator Label")
364364
self.assertEqual(new_label.creator_id, self.other_user.id)
365+
366+
def test_blank_text_is_rejected(self) -> None:
367+
"""An empty/whitespace ``text`` must NOT silently fall back to the
368+
``"Text Label"`` model default — clients should see a validation error.
369+
"""
370+
371+
result = self.client.execute(
372+
CREATE_LABEL_FOR_LABELSET_MUTATION,
373+
variables=self._create_variables(" "),
374+
)
375+
376+
self.assertIsNone(result.get("errors"))
377+
data = result["data"]["createAnnotationLabelForLabelset"]
378+
self.assertFalse(data["ok"])
379+
self.assertIn("blank", data["message"].lower())
380+
self.assertEqual(self.labelset.annotation_labels.count(), 0)

0 commit comments

Comments
 (0)