fix: disallow passing empty hosts to _UriData and fetch-libs at build time
#6
Conversation
jedel1043
force-pushed
the
fix-uri-data
branch
2 times, most recently
from
691c717 to
9ff36a8
Compare
| for host in hosts: | ||
| if not host: | ||
| raise FilesystemInfoError(f"invalid host `{host}`") |
There was a problem hiding this comment.
I'm a bit confused what this is doing here? I'm inclined to believe that we're filtering out falsey conditions such as empty strings "" and None, however, doesn't this mean that integers, floats, and True will be treated as valid hostnames as well? Not sure if this is behavior that we want. If we're just trying to quickly validate what's being passed, we could possibly use jsonschema or pydantic.
Personally pydantic is a bit overkill imho, but jsonschema could solve our problem nicely here:
{
"$schema": "https://json-schema.org/draft/2020-12/schema",
"type": "object",
"properties": {
"scheme": {"type": "string"},
"hosts": {"type": "array", "items": {"type": "string"}, "uniqueItems": True},
"user": {"type": "array", "items": {"type": "string"}, "uniqueItems": True},
"path": {"type": "string"},
"options": {"type": "object", ...},
},
"additionalProperties": False,
}The initialization arguments should be passed as the correct type and/or structure to _UriData rather than put the onus on us to beat the passed values into the desired format that we want. It makes the constructor much simpler, and requires much less edge case handling.
There was a problem hiding this comment.
I really don't want to rely on PYDEPS though, since we don't really know if Charmcraft will decide to deprecate it in the future...
There was a problem hiding this comment.
That's fair reasoning for not wanting to use jsonschema directly, but I still believe that we shouldn't carry a bunch of code for performing custom type validation at runtime. I think it's fair for us to expect that the arguments provided when a _UriData object is created are of the correct type. This way we potentially don't need to override the __init__ method for the dataclass. IIRC pyright should ideally catch when a dev is passing a value of the wrong type to _UriData.
If we do still want runtime checking, I still think jsonschema would be useful. I've found that custom validators can get messy quick especially if you don't have tight control over the input data.
There was a problem hiding this comment.
I agree. I'll remove the validation code.
jedel1043
force-pushed
the
fix-uri-data
branch
from
9ff36a8 to
65308a1
Compare
| for host in hosts: | ||
| if not host: | ||
| raise FilesystemInfoError(f"invalid host `{host}`") |
There was a problem hiding this comment.
That's fair reasoning for not wanting to use jsonschema directly, but I still believe that we shouldn't carry a bunch of code for performing custom type validation at runtime. I think it's fair for us to expect that the arguments provided when a _UriData object is created are of the correct type. This way we potentially don't need to override the __init__ method for the dataclass. IIRC pyright should ideally catch when a dev is passing a value of the wrong type to _UriData.
If we do still want runtime checking, I still think jsonschema would be useful. I've found that custom validators can get messy quick especially if you don't have tight control over the input data.
jedel1043
force-pushed
the
fix-uri-data
branch
from
65308a1 to
a506cd0
Compare
Fixes a bug that allowed passing empty host names to
_UriDatafrom theNfsInfoclass.Fixes the charmcraft build.