Allow hash keys to be URIs that dereference to a string

[joachimvh]

Closes https://github.com/LinkedSoftwareDependencies/Components.js/issues/115

Reason this already worked for values is because the argsCreator was called to determine the actual value of the value: https://github.com/LinkedSoftwareDependencies/Components.js/blob/10fb45d890d6d5095a7258183cb633d35735292c/lib/construction/argument/ArgumentConstructorHandlerHash.ts#L36-L40

I applied the same logic to the key.

[coveralls]

Pull Request Test Coverage Report for Build 4925872942

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 4 of 4 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage remained the same at 100.0%

---

Totals
Change from base Build 4506773039:	0.0%
Covered Lines:	1361
Relevant Lines:	1361

---

💛 - Coveralls

[rubensworks]

Released as 5.4.0.

[joachimvh]

I have looked into why this caused issues and had to be reverted. Also why it did not cause issues in all cases.

The short answer is, I'm pretty sure just changing the line here to not have quotes fixes the issue: https://github.com/LinkedSoftwareDependencies/Components.js/blob/8934ec9b784def601730b3d3f2e60c4ff0b8776e/lib/construction/strategy/ConstructionStrategyCommonJsString.ts#L97

The problem is that originally, the ArgumentConstructorHandlerHash returned an object with key being whatever was in the value field of a Term, and value containing the instantiation of a Term. This PR changed it so key also contained the instantiation of this term.

Now, why does this cause an issue? The reason is that to instantiate a Term that corresponds to a string (or other primitive), the ArgumentConstructorHandlerPrimitive is called, which then calls the createPrimitive function of the construction strategy. The same construction strategy that generates invalid data in the getHash function above. As you can see here, the issue is that this createPrimitive function adds quotes around the output value: https://github.com/LinkedSoftwareDependencies/Components.js/blob/8934ec9b784def601730b3d3f2e60c4ff0b8776e/lib/construction/strategy/ConstructionStrategyCommonJsString.ts#L134

As to the question why this does not happen in some cases, my assumption is that it is because of the fact that there are 2 construction strategies. The createPrimitive function in the ConstructionStrategyCommonJs does not add quotes like this and just returns the value of the Term, so there the output would be the same as it was before this PR: https://github.com/LinkedSoftwareDependencies/Components.js/blob/8934ec9b784def601730b3d3f2e60c4ff0b8776e/lib/construction/strategy/ConstructionStrategyCommonJs.ts#L105

I don't know in which case which strategy gets used so I can't say for sure that the above is correct.

[rubensworks]

Ah, as createPrimitive in CommonJsString includes the quotes, removing the quoted from getHash should resolve that problem indeed.

I remember there also being a problem in comunica related to hashes without config compilation (so just running the engine dynamically from the config). I'm not sure if this problem will be resolved through this change in CommonJsString, so another change might be needed.

[joachimvh]

I remember there also being a problem in comunica related to hashes without config compilation (so just running the engine dynamically from the config). I'm not sure if this problem will be resolved through this change in CommonJsString, so another change might be needed.

I don't fully understand this. Is this a problem that occurs because of this PR or something else? In other words, should I open a new PR with the changes of this one with the addition of removing the quotes, or does something else also need to be tested?

[rubensworks]

Yes, this problem was also introduced because of this PR.

Feel free to open a new PR with the changes of this one. But we should then test it in comunica, both with config compilation and dynamically starting the engine from the (default) config.