String xsd:double value crash toRdf()

[anatoly-scherbakov]

Why

Here's a JSON-LD document.

{
  "@context": {
    "xsd": "http://www.w3.org/2001/XMLSchema#",
    "geoLongitude": "http://www.w3.org/2003/01/geo/wgs84_pos#longitude"
  },
  "@graph": [
    {
      "@id": "http://www.wikidata.org/entity/Q399",
      "geoLongitude": {
        "@type": "xsd:double",
        "@value": "45"
      }
    }
  ]
}

Conversion of this document to a RDF dataset works at JSON-LD playground, but it will crash pyld 2.0.4:

    def _object_to_rdf(self, item, issuer, triples, rdfDirection):
        """
        Converts a JSON-LD value object to an RDF literal or a JSON-LD string
        or node object to an RDF resource.
    
        :param item: the JSON-LD value or node object.
        :param issuer: the IdentifierIssuer for issuing blank node identifiers.
        :param triples: the array of triples to append list entries to.
        :param rdfDirection: for creating datatyped literals.
        :param rdfDirection: for creating datatyped literals.
    
        :return: the RDF literal or RDF resource.
        """
        object = {}
    
        if _is_value(item):
            object['type'] = 'literal'
            value = item['@value']
            datatype = item.get('@type')
    
            # convert to XSD datatypes as appropriate
            if item.get('@type') == '@json':
                object['value'] = canonicalize(value).decode('UTF-8')
                object['datatype'] = RDF_JSON_LITERAL
            elif _is_bool(value):
                object['value'] = 'true' if value else 'false'
                object['datatype'] = datatype or XSD_BOOLEAN
            elif _is_double(value) or datatype == XSD_DOUBLE:
                # canonical double representation
                object['value'] = re.sub(
                    r'(\d)0*E\+?0*(\d)', r'\1E\2',
>                   ('%1.15E' % value))
E               TypeError: must be real number, not str

What

This PR

• Creates a unit test demonstrating the problem,
• Contains a fix to it.

[anatoly-scherbakov]

@BigBlueHat @davidlehn please let me know what you think. Thanks!

[davidlehn]

• The code style is different. Extra newlines and such. Would be better to match what is there.
• None of the other code has type annotations. (This code pre-dates that even existing.) I'm not sure if we want to add that in new code piece by piece or add it all at once later.
• Does the test suite not cover this? If not, it should, so all implementations test this case. That might remove the need for the test file.

[anatoly-scherbakov]

1. I agree about importance of maintaining a unified code style. However, both describing such as style and mimicking it, are rather difficult if done informally. An automatic formatter, such as black, is often considered a fit resolution to such issues (even though I dislike certain decisions black does, I have to agree its benefits are worthwhile). We might consider introducing it to the project and to CI.

2. This is a matter of taste; IMHO type annotations are de facto standard in industry programming in Python, and I tend to use them in every piece of code I write, not even questioning whether I should, I am afraid. A typechecker can be introduced into development workflow and in CI to validate type annotations.

3. The purpose of the test is to a) illustrate the existence of the issue and b) the ability of the proposed fix to remedy it. Writing a test was a part of my debugging process when I discovered the problem when parsing a document from Wikidata, and I included the test as a material proof.

An extra test should not bring about much harm. It can be deduplicated and, if necessary, merged into the test suite of the API specification later.

[davidlehn]

Conversion of this document to a RDF dataset works at JSON-LD playground, but it will crash pyld 2.0.4:

Please don't assume "it works in the json-ld.org playground" means the output is correct! We need the shared tests to be reviewed to ensure they are actually correct. It's entirely possible that the playground js code is doing something wrong and needs fixing too. Better to have the test in the approved shared suite so everyone agrees on behavior.

1. I agree about importance of maintaining a unified code style. [...]

Great, please update the patch. Looks like it's probably a few blank lines, trailing commas, and some trailing whitespace to fix?

Open an issue and/or PR about using other tooling.

2. This is a matter of taste; [...]

Again, seems like this patch should match the style of the existing code, which doesn't yet use annotations. Probably best to discuss it in an issue then slowly or bulk add everywhere needed.

3. The purpose of the test is to a) illustrate the existence of the issue and b) the ability of the proposed fix to remedy it. Writing a test was a part of my debugging process when I discovered the problem when parsing a document from Wikidata, and I included the test as a material proof.

I wasn't questioning the need or reason to have a test. Did you determine if the test suite had anything to cover this? That a test needed to be written seems to imply no. This 100% needs to be in the spec api test suite if that is the case. All implementations need to cover the issue.

I know it's a bit awkward to figure out how to add tests to the spec suite. Naming there has been a mess. But adding something that can get some feedback and adjusted later is not too bad.

At least in the js implementation there are (clunky) features to run single or a group of tests. I use that all the time. It's often as easy as local per-implementation tests and the general tests need to be in the api test suite anyway.

An extra test should not bring about much harm.

It is absolutely harmful to have general tests that only exist in one implementation! Interoperability is what JSON-LD is about. All implementations need to know they function the same on a test suite shared and approved by the community.

It can be deduplicated and, if necessary, merged into the test suite of the API specification later.

Duplication is a problem. It would mix up which tests are Python specific vs ones that need to be added to the API test suite and removed. If it is a duplicate, than please clearly document why and refer to the API test. But what would that be for?

It's now later and it is necessary. Please add it to the spec api test suite.

I also find adding to the test suite sometimes encourages me or others to add more variations. Especially when it's about a handful of triples. Like here, do positive, negative, zero, lots of sig digits, values near maximums and minimums, maybe some non-numbers if it makes sense, etc.

All that being said, thank you for the test and fix. :-)

[mielvds]

I included a compromise of this in #228 based on the work & comments of @anatoly-scherbakov @davidlehn. Discussion can continue there if desired.