Implementation necessary to achieve use case 3 - obtaining events

[alien-mcl]

Please find this PR approaching use case 3 - obtaining events.
While the changes in the implementation are relatively small, this PR should open a discussion on how the public API should look like.
I personally prefer a bag of hypermedia controls from which developer can filter out what he needs. Alternative is to have specialized methods that will pick i.e. links or operations.
Drawback of the latter from my perspective is that we may find nuances that link may be considered a special kind of operation or other interesting observations.
Also suggested implementation uses the client instance directly to obtain resources - alternative is to have hypermedia controls to expose methods for doing so.

---

This change is 

[alien-mcl]

It seems this PR went unnoticed. Is there any criticism about changes provided?

[lanthaler]

I was on vacation, too bad no one else picked it up in the meantime. Feel free to ping the mailing list for each new PR. It might be that most people still aren't subscribed to the repo and thus won't get a notification.

---

Review status: 0 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

integration-tests/HydraClient.spec.ts, line 37 at r1 (raw file):

                beforeEach(run(async function () {
                    this.events = await this.client.getResource(this.entryPoint.hypermedia
                        .find(link => ~link.isA.indexOf(hydra.Collection)).iri);

What is this tilde (~) for here?

---

integration-tests/HydraClient.spec.ts, line 38 at r1 (raw file):

                    this.events = await this.client.getResource(this.entryPoint.hypermedia
                        .find(link => ~link.isA.indexOf(hydra.Collection)).iri);
                    this.members = this.events.hypermedia.find(link => ~link.isA.indexOf(hydra.Collection)).members;

This interface seems very un-intuitive to me. As a user of Heracles I'd expect to be able to do something like this.events.get(hydra.member) or even this.events.getMembers()

---

integration-tests/HydraClient.spec.ts, line 42 at r1 (raw file):

                it("should obtain a collection of events", function () {
                    expect(this.members).toEqual([{ iri: this.url + "api/events/1" }]);

Given the description, we should probably type the members and check whether there are a few events instead of looking for a specific URL.

---

integration-tests/server/api/events.jsonld, line 6 at r1 (raw file):

    "@type": "hydra:Collection",
    "member": [
        { "@id": "/api/events/1" }

As already mentioned in the test, I think we should type this and perhaps add 1 or 2 more members.

---

src/DataModel/JsonLd/JsonLdHypermediaProcessor.ts, line 140 at r1 (raw file):

        for (let resource of result)
        {
            JsonLdHypermediaProcessor.fixTypeOf(resource);

Did you forget to remove the method above or the code below? You are doing the same thing twice.

I would also recommend to rename this method to something like makeTypeArray or something similar that is a bit more descriptive.

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 34 at r1 (raw file):

            }));

            it("should process data", function() {

Is this nesting off? You seem to close the describe block above

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 39 at r1 (raw file):

            it("should separate hypermedia", function() {
                expect(this.result.hypermedia).toEqual([

I think this shows the leaky nature of this interface quite clearly. It returns a more or less random set of data. I'd expect to be able to get a list of links to related resources and a list of operations that I then can invoke.

Something like

links = this.result.getLinks();
--> returns an array of Link instances which have the property, the target 
    (and perhaps a few additional properties describing the target)
links[0].getProperty() --> examplevocab:events
links[0].getTarget() --> /api/events
...

This link could then simply be passed to the client this.client.getResource(links[0]) or, perhaps more intuitive, links[0].retrieveTarget(this.client)

---

Comments from Reviewable

[alien-mcl]

No problem - I expected it somehow

---

Review status: 0 of 6 files reviewed at latest revision, 7 unresolved discussions.

---

integration-tests/HydraClient.spec.ts, line 37 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

What is this tilde (~) for here?

Nicely converts result of the indexOf [-1; n] method into a boolean

---

integration-tests/HydraClient.spec.ts, line 38 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

This interface seems very un-intuitive to me. As a user of Heracles I'd expect to be able to do something like this.events.get(hydra.member) or even this.events.getMembers()

It was the cheapest. As for your example - event is the original resource - we agreed that by default we don't modify it. I'd put it to the hypermedia property. As for the members, I think get(hydra.member) seems more generic as hypermedia is a bag of various objects.

---

integration-tests/HydraClient.spec.ts, line 42 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Given the description, we should probably type the members and check whether there are a few events instead of looking for a specific URL.

Good point

---

integration-tests/server/api/events.jsonld, line 6 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

As already mentioned in the test, I think we should type this and perhaps add 1 or 2 more members.

You mean have it inlined in assertions?

---

src/DataModel/JsonLd/JsonLdHypermediaProcessor.ts, line 140 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Did you forget to remove the method above or the code below? You are doing the same thing twice.

I would also recommend to rename this method to something like makeTypeArray or something similar that is a bit more descriptive.

Indeed, something's redundant here

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 34 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Is this nesting off? You seem to close the describe block above

I'm not sure I get it

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 39 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

I think this shows the leaky nature of this interface quite clearly. It returns a more or less random set of data. I'd expect to be able to get a list of links to related resources and a list of operations that I then can invoke.

Something like

links = this.result.getLinks();
--> returns an array of Link instances which have the property, the target 
    (and perhaps a few additional properties describing the target)
links[0].getProperty() --> examplevocab:events
links[0].getTarget() --> /api/events
...

This link could then simply be passed to the client this.client.getResource(links[0]) or, perhaps more intuitive, links[0].retrieveTarget(this.client)

I'll think it over

---

Comments from Reviewable

[lanthaler]

Reviewed 6 of 6 files at r1.
Review status: all files reviewed at latest revision, 8 unresolved discussions.

---

integration-tests/HydraClient.spec.ts, line 37 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Nicely converts result of the indexOf [-1; n] method into a boolean

Could we please make this more explicit? The tilde is a bitwise NOT operator so -1 becomes 0, 0 becomes -1, 1 becomes -2.. not easy to follow

---

integration-tests/HydraClient.spec.ts, line 38 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

It was the cheapest. As for your example - event is the original resource - we agreed that by default we don't modify it. I'd put it to the hypermedia property. As for the members, I think get(hydra.member) seems more generic as hypermedia is a bag of various objects.

Sure, it's more generic but is that a problem? I understand that it is more difficult to implement but it makes the use of this library much easier.. I'm fine to leave that to a separate PR though if you prefer

---

integration-tests/server/api/events.jsonld, line 6 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

You mean have it inlined in assertions?

I meant changing it to something like

"member": [
        { "@id": "/api/events/1", "@type": "Event" },
        { "@id": "/api/events/2", "@type": "Event" },
        { "@id": "/api/events/3", "@type": "Event" }

---

src/DataModel/JsonLd/context.json, line 26 at r1 (raw file):

        },
        "http://www.w3.org/ns/hydra/core#Collection": "http://www.w3.org/ns/hydra/core#Collection",
        "http://www.w3.org/ns/hydra/core#Resource": "http://www.w3.org/ns/hydra/core#Resource"

I assume this should be

        "Collection": "hydra:Collection",
        "Resource": "hydra:Resource"

instead

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 34 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I'm not sure I get it

Sorry, my bad.. I missed the run and inside the it block. I thought you close the describe block.

---

Comments from Reviewable

[elf-pavlik]

integration-tests/HydraClient.spec.ts, line 37 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Could we please make this more explicit? The tilde is a bitwise NOT operator so -1 becomes 0, 0 becomes -1, 1 becomes -2.. not easy to follow

stepping aside from the code style aspect, it looks like client just picks any first collection that appears in the entry point with .find(link => ~link.isA.indexOf(hydra.Collection)).iri), i think test should include at least one other collection (eg. '/api/people') so that client needs to discover the desired collection
if this PR doesn't want to address discovery, maybe it should just hard code /api/events just as "should obtain a collection of events" does

---

Comments from Reviewable

[alien-mcl]

Review status: 2 of 13 files reviewed at latest revision, 7 unresolved discussions.

---

integration-tests/HydraClient.spec.ts, line 37 at r1 (raw file):

Previously, elf-pavlik (elf Pavlik) wrote…

stepping aside from the code style aspect, it looks like client just picks any first collection that appears in the entry point with .find(link => ~link.isA.indexOf(hydra.Collection)).iri), i think test should include at least one other collection (eg. '/api/people') so that client needs to discover the desired collection
if this PR doesn't want to address discovery, maybe it should just hard code /api/events just as "should obtain a collection of events" does

Ok, I've removed this pseudo-discovery and replaced it with explicit resource Url.

---

integration-tests/HydraClient.spec.ts, line 38 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Sure, it's more generic but is that a problem? I understand that it is more difficult to implement but it makes the use of this library much easier.. I'm fine to leave that to a separate PR though if you prefer

I've added resource.hypermedia.members with option to expand with other accessors.

---

integration-tests/HydraClient.spec.ts, line 42 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Good point

Replaced with typed member check

---

integration-tests/server/api/events.jsonld, line 6 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

I meant changing it to something like

"member": [
        { "@id": "/api/events/1", "@type": "Event" },
        { "@id": "/api/events/2", "@type": "Event" },
        { "@id": "/api/events/3", "@type": "Event" }

I've added types and two more members

---

src/DataModel/JsonLd/context.json, line 26 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

I assume this should be

        "Collection": "hydra:Collection",
        "Resource": "hydra:Resource"

instead

Ahh. It was some time ago, but I think this was to force either framing or flattening to enforce those urls. For some reason these were incorrectly processed.

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 34 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

Sorry, my bad.. I missed the run and inside the it block. I thought you close the describe block.

Ok

---

Comments from Reviewable

[lanthaler]

Reviewed 9 of 11 files at r2, 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

integration-tests/HydraClient.spec.ts, line 38 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I've added resource.hypermedia.members with option to expand with other accessors.

OK. I'm still not very happy about this hypermedia property but let's keep it as is for now and move on.

---

integration-tests/server/api/events.jsonld, line 6 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I've added types and two more members

Thanks

---

src/DataModel/JsonLd/context.json, line 26 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Ahh. It was some time ago, but I think this was to force either framing or flattening to enforce those urls. For some reason these were incorrectly processed.

I don't follow. What's the point of mapping these URLs to themselves?

---

Comments from Reviewable

[elf-pavlik]

src/DataModel/JsonLd/context.json, line 26 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

I don't follow. What's the point of mapping these URLs to themselves?

It looks that in rest of the codebase you always import { hydra } from './namespaces' so JSON-LD context used by the client doesn't need to create any aliases. How about just removing those last two line all together?
A future PR could propose any further improvements on what uses namespaces module and what relies on aliases in JSON-LD context.

---

Comments from Reviewable

[elf-pavlik]

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/DataModel/JsonLd/JsonLdHypermediaProcessor.ts, line 140 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Indeed, something's redundant here

I think you might have extracted fixTypeOf from fixType but didn't delete extracted code after calling fixTypeOf inside of fixType.
BTW in cases other than @type JSON-LD context seems to rely on "@container": "@set"to enforce array. It does not seem possible to do something similar for @type

    "isA": {
      "@id": "@type",
      "@container": "@set"
    }

---

Comments from Reviewable

[alien-mcl]

Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

integration-tests/HydraClient.spec.ts, line 38 at r1 (raw file):

Previously, lanthaler (Markus Lanthaler) wrote…

OK. I'm still not very happy about this hypermedia property but let's keep it as is for now and move on.

OK

---

src/DataModel/JsonLd/context.json, line 26 at r1 (raw file):

Previously, elf-pavlik (elf Pavlik) wrote…

It looks that in rest of the codebase you always import { hydra } from './namespaces' so JSON-LD context used by the client doesn't need to create any aliases. How about just removing those last two line all together?
A future PR could propose any further improvements on what uses namespaces module and what relies on aliases in JSON-LD context.

As for those two full urls - I'm using a hydra's Url as @vocab which causes hydra based types to be shortened to Collection or Resource. Still, while processing full urls are preferred, thus that's why I'm forcing those in the context.
As for the namespace imports - I'm not sure what does it have to do with JSON-LD processing - imports are just code imports.

---

src/DataModel/JsonLd/JsonLdHypermediaProcessor.ts, line 140 at r1 (raw file):

Previously, elf-pavlik (elf Pavlik) wrote…

I think you might have extracted fixTypeOf from fixType but didn't delete extracted code after calling fixTypeOf inside of fixType.
BTW in cases other than @type JSON-LD context seems to rely on "@container": "@set"to enforce array. It does not seem possible to do something similar for @type

    "isA": {
      "@id": "@type",
      "@container": "@set"
    }

Yeah - I've tried using @set for @type. I've raised it at digitalbazaar/jsonld.js#191 (unsure whether it is implementation or specification issue). BTW - I forgot to fix - I'll try to update PR ASAP.

---

Comments from Reviewable

[elf-pavlik]

---

Review status: 12 of 13 files reviewed at latest revision, 3 unresolved discussions.

---

src/DataModel/JsonLd/context.json, line 26 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

As for those two full urls - I'm using a hydra's Url as @vocab which causes hydra based types to be shortened to Collection or Resource. Still, while processing full urls are preferred, thus that's why I'm forcing those in the context.
As for the namespace imports - I'm not sure what does it have to do with JSON-LD processing - imports are just code imports.

I think we shouldn't let this block this PR so I captured it in separate issue #15

---

src/DataModel/JsonLd/JsonLdHypermediaProcessor.ts, line 140 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

Yeah - I've tried using @set for @type. I've raised it at digitalbazaar/jsonld.js#191 (unsure whether it is implementation or specification issue). BTW - I forgot to fix - I'll try to update PR ASAP.

regarding changing naming of this function, maybe we could propose fine tuning it after merging this PR with a dedicated PR?

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 39 at r1 (raw file):

Previously, alien-mcl (Karol Szczepański) wrote…

I'll think it over

can we possibly capture it into a separate issue so that we can merge this PR and keep iterating?

---

Comments from Reviewable

[lanthaler]

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

src/DataModel/JsonLd/JsonLdHypermediaProcessor.ts, line 140 at r1 (raw file):

Previously, elf-pavlik (elf Pavlik) wrote…

regarding changing naming of this function, maybe we could propose fine tuning it after merging this PR with a dedicated PR?

Using @set for @type doesn't work as JSON-LD doesn't allow you to change the behavior of its keywords.

Tweaking the naming in a dedicated PR sounds good to me.

---

tests/DataModel/JsonLd/JsonLdMetadataProvider.spec.ts, line 39 at r1 (raw file):

Previously, elf-pavlik (elf Pavlik) wrote…

can we possibly capture it into a separate issue so that we can merge this PR and keep iterating?

Sure. Filed issue #16 for this

---

Comments from Reviewable

[lanthaler]

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[tpluscode]

---

Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable