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.

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

Add validation checks in Jurisdiction constructor #40

Conversation

carbonphyber
Copy link
Contributor

@carbonphyber carbonphyber commented Mar 7, 2021

Add validation of parameters to Jurisdiction constructor. The constructor now raises Exceptions where it didn't before (BREAKING CHANGE).

This PR prevents all instances of Jurisdiction from working with any hostname other than results.enr.clarityelections.com. This will cause breakage if any downstream users depend on any other hostnames/CNAMEs (potential BREAKING CHANGE). I couldn't find any documentation about Clarify about whether they support such features or not.

This PR also restricts the valid values of level to ['state', 'county', 'city'] (BREAKING CHANGE). Are there any other valid values?

@ghing
Copy link
Contributor

ghing commented Mar 7, 2021

I haven't had a chance to fully think through which values should be accepted, but I think that when doing these checks, it makes more sense to raise a TypeError (when checking whether the URL is string-like) or ValueError (when checking the URL format and the value of level) instead of just Exception.

I also think that checking for strings explicitly is going to cause compatibility problems because there are probably string-like objects that should be supported for which type(url) will not return str. I think it would be more Pythonic to wrap the place where the URL is used as a string in a try/except and to raise a ValueError if the value can't be used as a string. In fact, the functions that are called may already do this and we'd only want to rethrow an error if the existing ValueError message is unclear.

… conversion to str in Jusidsiction constructor, catching Exceptions; additional unit tests
…iction constructor while converting url to str
@carbonphyber
Copy link
Contributor Author

carbonphyber commented Mar 8, 2021

it makes more sense to raise a TypeError (when checking whether the URL is string-like) or ValueError (when checking the URL format and the value of level) instead of just Exception.

Done.

I also think that checking for strings explicitly is going to cause compatibility problems because there are probably string-like objects that should be supported for which type(url) will not return str. I think it would be more Pythonic to wrap the place where the URL is used as a string in a try/except and to raise a ValueError if the value can't be used as a string. In fact, the functions that are called may already do this and we'd only want to rethrow an error if the existing ValueError message is unclear.

I followed your logic and converted the url parameter in the constructor to a str. If that raises, the constructor raises a TypeError. If after that conversion to str doesn't match the URL pattern, we throw a ValueError. Unit tests added for both classes which implement __str__ and classes which don't.

Reraises TypeError, ValueError if thrown in Jurisdiction constructor while converting url to str.

Also worth noting that level now accepts 'precinct' as a valid value for level since a Parser unit test uses it.

@dwillis dwillis merged commit 0510edd into openelections:master Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants