feat: add permissive UID validation for DicomWebClient

[jaya-krishnan-sr]

Related Issue

Fixes #117

Changes

• Add permissive parameter to DICOMwebClient class
• Update the default regex to be more in line with URI class
• Add minimal units to test the changes

[CPBridge]

Thanks @jaya-krishnan-sr

A couple of thoughts:

1. It seems that you have essentially copy-pasted the relevant code from uri.py into web.py. Why not import it (i.e. _validate_uid) instead and reduce redundancy?
2. I think permissive is too vague a name for the parameter of the client constrcutor. It makes sense for the _validate_uid function, where the meaning is clear from context. But for the client itself, it could mean that all sorts of things are permissive. How about changing the name of the client constructor parameter to permissive_uid?

[jaya-krishnan-sr]

Thanks for the review @CPBridge

1. It seems that you have essentially copy-pasted the relevant code from uri.py into web.py. Why not import it (i.e. _validate_uid) instead and reduce redundancy?

Theres an additional MAX_UID_LENGTH check present as part of the _validate_uid in uri.py. I was worried about introducing this additional validation in web.py. If thats not a problem, I can update the code to reuse the same method.

2. I think permissive is too vague a name for the parameter of the client constrcutor. It makes sense for the _validate_uid function, where the meaning is clear from context. But for the client itself, it could mean that all sorts of things are permissive. How about changing the name of the client constructor parameter to permissive_uid?

Yup. Agreed 👍🏽

[CPBridge]

Theres an additional MAX_UID_LENGTH check present as part of the _validate_uid in uri.py. I was worried about introducing this additional validation in web.py. If thats not a problem, I can update the code to reuse the same method.

Makes sense. But I think this should be fine, especially now that the permissive_uid parameter can be used to override it

[jaya-krishnan-sr]

Makes sense. But I think this should be fine, especially now that the permissive_uid parameter can be used to override it

Unfortunately the permissive flag will not override this _MAX_UID_LENGTH check.

dicomweb-client/src/dicomweb_client/uri.py

Lines 630 to 640 in ded157f

	def _validate_uid(uid: str, permissive: bool) -> None:
	"""Validates a DICOM UID."""
	if len(uid) > _MAX_UID_LENGTH:
	raise ValueError('UID cannot have more than 64 chars. '
	f'Actual count in {uid!r}: {len(uid)}')
	if not permissive and _REGEX_UID.fullmatch(uid) is None:
	raise ValueError(f'UID {uid!r} must match regex {_REGEX_UID!r} in '
	'conformance with the DICOM Standard.')
	elif permissive and _REGEX_PERMISSIVE_UID.fullmatch(uid) is None:
	raise ValueError(f'Permissive mode is enabled. UID {uid!r} must match '
	f'regex {_REGEX_PERMISSIVE_UID!r}.')

[CPBridge]

Ah okay. Well I don't think there's a good reason for that. I would prefer that we use the same method, and have the permissive flag skip the length check.

[CPBridge]

One tiny tweak and we are good to go, thanks @jaya-krishnan-sr

I will put out a patch release for this once merged

[CPBridge]

Hi @jaya-krishnan-sr sorry can you also fix the linter errors?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[jaya-krishnan-sr]

Hi @jaya-krishnan-sr sorry can you also fix the linter errors?

Hey @CPBridge, I have fixed all the linter errors. But i could see some mypy errors in existing code. If these are coming up in the CI too, I am happy to resolve them 🙂

[CPBridge]

Looks good