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

ExternalReference url parameters that are not XsUri cause hard to find serialization issues #363

Closed
schlenk opened this issue Mar 22, 2023 · 7 comments

Comments

@schlenk
Copy link
Contributor

schlenk commented Mar 22, 2023

The 4.0 version is nice, but has changed a few things in the serialization.

In 3.x one could use:

            ExternalReference(
                type=ExternalReferenceType.WEBSITE,
                url="https://example.org",
            )

In 4.0, this needs some boilerplate XsUri:

            ExternalReference(
                type=ExternalReferenceType.WEBSITE,
                url=XsUri("https://example.com"),
            )

Omitting the XsUri in 4.0 this causes a Traceback on serialization:

File "c:\code\.venv\Lib\site-packages\cyclonedx\output\__init__.py", line 85, in output_to_file
    f_out.write(self.output_as_string().encode('utf-8'))
                ^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\code\.venv\Lib\site-packages\cyclonedx\output\xml.py", line 70, in output_as_string
    self.generate()
  File "c:\code\.venv\Lib\site-packages\cyclonedx\output\xml.py", line 63, in generate
    self._root_bom_element = self.get_bom().as_xml(  # type: ignore
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\code\.venv\Lib\site-packages\serializable\__init__.py", line 400, in _as_xml
    nested_e.append(j.as_xml(view_=view_, as_string=False, element_name=nested_key, xmlns=xmlns))
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\code\.venv\Lib\site-packages\serializable\__init__.py", line 400, in _as_xml
    nested_e.append(j.as_xml(view_=view_, as_string=False, element_name=nested_key, xmlns=xmlns))
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "c:\code\.venv\Lib\site-packages\serializable\__init__.py", line 425, in _as_xml
    this_e.append(v.as_xml(view_=view_, as_string=False, element_name=new_key, xmlns=xmlns))
                  ^^^^^^^^
AttributeError: 'str' object has no attribute 'as_xml'

It would be nice, if the url parameters, especially for ExternalReference check the type of the argument. The code has Mypy type annotations listing XsUri, yes, but that does not help to figure it out at runtime.

@jkowalleck
Copy link
Member

you are right about the type.
It is properly documented here:

def __init__(self, *, type: ExternalReferenceType, url: XsUri, comment: Optional[str] = None,
hashes: Optional[Iterable[HashType]] = None) -> None:
self.url = url
self.comment = comment
self.type = type
self.hashes = hashes or [] # type: ignore

Python is not hard typed.
And there is no intention to add hard type checks in the library, if not necessary.

What prevents from passing the correct data type (XsUri, not str) to the ExternalReference?

@schlenk
Copy link
Contributor Author

schlenk commented Mar 22, 2023

Nothing prevents that. So it is trash in -> trash out of course.

It is just some not so nice user API design that the 'url' parameter requires XsUri and not just str. I can see why this is done to make the code nicely orthogonal.

The serialization blows up with the unhelpful Exception in that case. Some better diagnostics at serialization time would be nice, to discover the part of the model that is in an invalid state.

So feel free to close this, if better diagnostics is out of scope.

@jkowalleck
Copy link
Member

It is just some not so nice user API design that the 'url' parameter requires XsUri and not just str.

how would a "nice user API design" look like?
What change do you suggest? Maybe you could draft a backwards-compatible change request, to show?

@schlenk
Copy link
Contributor Author

schlenk commented Mar 22, 2023

I migrated some code, and had a few instances of this pattern setting optional URLs, e.g. in License.

           License(
                ...
                url=somedict.get('URL'),
            )

this works nicely when url takes a str, as in Optional[str] and keeps XsUri as an implementation detail thats just a fancy validator, but needs some extra None checks when an XsUri wrapper is needed. e.g. it mutates to something ugly like:

           License(
                ...
                url=XsUri(somedict['URL']) if somedict.get('URL') else None
            )

In addition most external users of the class probably do not care about the XsUri type for an URL. Typically they will have some form of string at hand. On reading out the URL, they will cast that away to str to feed it into other APIs. On writing they need to add boilerplace XsUri to convert the str values they have.

The only part needing that type is the serialization code and the validation step. So for most purposes visible to users of the class, the url parameter type could just be a type alias for str instead of a full blown class wrapper. The XsUri class should be an implementation detail, not a user visible part of the API.

e.g.

from typing import TypeAlias
urlstr: TypeAlias str

def __init__(self, *, type: ExternalReferenceType, url: urlstr, comment: Optional[str] = None, 
              hashes: Optional[Iterable[HashType]] = None) -> None: 
     self.url = url if isinstance(url, XsUri) else XsUri(str)
     self.comment = comment 
     self.type = type 
     self.hashes = hashes or []  # type: ignore 

the setter/getters would obviously need to do something as well. But using the code gets nicer. If i pass in junk, it raises the proper exceptions. I can directly pass values in from other places without value wrappers and so on.

This is a little different to the Purl case, where the PackageURL class actually carries more weight.

If you see merit in improving this, i can try to do a pull request.

@jkowalleck
Copy link
Member

jkowalleck commented Mar 23, 2023

so it would be more like (fixed typing)

from typing import Optional, Union
from ... import XsUri, ExternalReferenceType, HashType, Iterable


def __init__(self, *, 
             type: ExternalReferenceType, 
             url: Union[XsUri, str], 
             comment: Optional[str] = None, 
             hashes: Optional[Iterable[HashType]] = None
) -> None: 
  self.url = url if isinstance(url, XsUri) else XsUri(str)
  ...

would you create a pullrequest with the desired change?

@jkowalleck
Copy link
Member

see #432

@jkowalleck jkowalleck closed this as not planned Won't fix, can't repro, duplicate, stale Sep 22, 2023
@jkowalleck
Copy link
Member

jkowalleck commented Sep 22, 2023

closed.

downstream users may write themselves a wrapper, doing the exact same an in-bound code would do:
test if a URI is string, and if so then convert it to XsUri.

applies like (based on given examples above)

makeXsUriFromAny(v) -> XsUri | None:
  return XsUri(v) if isinstance(v, str) else None

License(
    ...
    url=makeXsUriFromAny(somedict['URL'])
)

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

No branches or pull requests

2 participants