feat(core): implement `exploreDirectory` method #1186

JKRhb · 2023-12-07T07:50:42Z

This PR starts implementing the exploreDirectory method that has recently been added to the Scripting API specification.

codecov · 2023-12-07T07:53:32Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d12f665) 76.65% compared to head (7cc777f) 77.60%.
Report is 17 commits behind head on master.

Files	Patch %	Lines
packages/core/src/validation.ts	96.15%	0 Missing and 1 partial ⚠️
packages/core/src/wot-impl.ts	98.03%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1186      +/-   ##
==========================================
+ Coverage   76.65%   77.60%   +0.95%     
==========================================
  Files          80       83       +3     
  Lines       16821    17311     +490     
  Branches     1619     1747     +128     
==========================================
+ Hits        12894    13435     +541     
+ Misses       3897     3840      -57     
- Partials       30       36       +6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

packages/core/src/wot-impl.ts

JKRhb · 2023-12-07T11:09:30Z

The current version of the implementation should now already be working – I performed some initial testing with the Zion directory :) Some details need to be fixed/improved, though, I will continue working on this later today.

This is required for fully supporting the `exploreDirectory` method. See https://www.w3.org/TR/wot-discovery/#exploration-directory-api-things-listing

JKRhb · 2023-12-14T11:19:15Z

I think this PR could now be ready for an initial review :) Even though there currently are some typescript quirks regarding the handling of ReadableStreams, interacting with a local instance of Zion to retrieve TDs already worked.

One major piece is currently missing the validation of the retrieved TDs – did we already have a function handy that could be used for that?

danielpeintner · 2023-12-15T09:41:29Z

One major piece is currently missing the validation of the retrieved TDs – did we already have a function handy that could be used for that?

ThingModelHelpers has isThingModel and validateThingModel. We might want to have something similar for ThingDescription.

danielpeintner

Do you see a possibility to have tests? Using Zion or so would require the service to be always up which may be an issue, right?

relu91 · 2023-12-15T11:22:39Z

Do you see a possibility to have tests? Using Zion or so would require the service to be always up which may be an issue, right?

Having a second look, doing tests with Zion means that we need to add them in the CLI because we would need to have also the binding templates up and running. It complicates a little bit the setup but it might be good to create a sort of end-to-end testing in a single place. In other words, we won't need to test the operations per binding but we could have a single code base in one place. Probably it is too much work for a single PR. What do you think?

JKRhb · 2023-12-15T11:59:47Z

I think for now I would try to mock the directory API, but having integration or end-to-end testing with Zion in place in the mid-term sounds like a great idea :)

danielpeintner · 2023-12-15T16:14:00Z

packages/core/src/validation.ts

+export function isThingDescription(input: unknown): input is WoT.ThingDescription {
+    // TODO: Improve validation function


Does it make sense to also have proper validation based on the JSON schema (AJV) validation like we do for TMs.

node-wot/packages/td-tools/src/thing-model-helpers.ts

Line 154 in 0fd864b

public static validateThingModel(data: ThingModel): { valid: boolean; errors?: string } {

Anyhow, definitely not in this PR and I am not even sure whether it makes sense int this case since we should be relaxed when consuming a TD and as strict as possible when creating a TD.

Thank you for your feedback! I actually had already started to change the validation approach so that it uses the JSON schema definition for validation and also added it to the PR now in the latest commits :) Hope that this is still fine

danielpeintner

I think this PR is a very good start and for me, it is okay to merge it as is 👍

danielpeintner

Looks still good 👍

relu91

I'm not sure if it is worth to approve the PR. Some remarks that I have:

validation: I like the approach but it is a half refactoring, we didn't consistently check where ajv was used and updated the code accordingly. I think the refactoring is needed (and has serious implications with Consume and read loop causes memory leak #1062) but it needs to be done systematically.
@ts-expect-error: I'm not comfortable about disabling tsc without a proper explanation of what is causing the problem and why is a false positive.

Nonetheless, with this new function we are still in the exploratory space so if @danielpeintner wants to move on I won't block it! Just keep in mind the two issues (that need to be added in the issue tracker).

danielpeintner · 2023-12-20T11:18:39Z

Nonetheless, with this new function we are still in the exploratory space so if @danielpeintner wants to move on I won't block it! Just keep in mind the two issues (that need to be added in the issue tracker).

@JKRhb @relu91 what do you think. Shall we tackle the issues mentioned by @relu91 in this PR or is it better to merge this PR as is and create follow-up PRs....

JKRhb · 2023-12-20T15:41:48Z

Thank you for your feedback, @relu91 and @danielpeintner! You raised valid concerns, @relu91, and I think we could provide some preliminary fixes in this PR:

validation: I like the approach but it is a half refactoring, we didn't consistently check where ajv was used and updated the code accordingly. I think the refactoring is needed (and has serious implications with Consume and read loop causes memory leak #1062) but it needs to be done systematically.

Yeah, I also had to think about the ajv issue, and fixing the underlying problem first definitely sounds like the better approach. As we are still in the experimental phase, I could just remove the body of the isThingDescription function or change it back to the very simple validation I had before (that is, only checking for the @context, the title, and the security definitions, for example). In a follow-up PR, we could then replace that validation logic with a more sophisticated one that might also utilize something like ajv.

@ts-expect-error: I'm not comfortable about disabling tsc without a proper explanation of what is causing the problem and why is a false positive.

Sure, I will investigate what causes the issue here :) It is a bit strange since it runs without a problem when ignoring the errors TypeScript is reporting here, but these might be related to the ES version we are targeting (note that I tried to use the "regular" streams instead of the Node.js ones). Should I try changing those to their Node.js counterpart?

JKRhb · 2023-12-21T10:32:11Z

I think @relu91's feedback should now be addressed for the most part – since Node.js streams seem to have a different API, I simplified the ThingDiscoveryProcess implementation a lot, making the use of the AsyncIterable interface almost a bit pointless. However, we might be able to utilize its asynchronous nature a bit better once we add an implementation for the discover method.

Regarding the ajv issue: I now reverted the change and implemented two wrapper functions around the Helper methods we already had in validation.ts that could be expanded upon in the future. I hope this is an improvement, otherwise just let me know :) I also applied the newly introduced functions in requestThingDescription by the way to ensure a unified behavior.

JKRhb · 2023-12-21T10:44:39Z

Added a tiny fix and two additional test cases in the latest commits.

Edit: With c8fc93f, these new tests are now also working as intended 😅

relu91

Thank you @JKRhb much better now!

relu91 · 2023-12-21T14:43:21Z

I think @relu91's feedback should now be addressed for the most part – since Node.js streams seem to have a different API

Btw have you tried the implementation in the browser? is browserify able to translate the streams correctly?

danielpeintner · 2023-12-21T15:37:40Z

packages/core/src/validation.ts

+export function getLastValidationErrors() {
+    const errors = Helpers.tsSchemaValidator.errors?.map((o: ErrorObject) => o.message).join("\n");
+    return new Error(errors);
+}


No strong opinion but the term might convey something which is not always the case. It is correct, it is the "last" validation. Anyhow, if used in some other code parts it might report errors of another validation step (not necessarily the one from last isThingDescription) that used Helpers.tsSchemaValidator..

Do you know what I mean.

danielpeintner · 2023-12-21T15:42:04Z

I think we can merge this PR as is and look at some points raised in the comments later

@relu91 check implementation in browser
@danielpeintner function getLastValidationErrors() not in any case what one would expect ...

Did I miss anything?
I created #1206 as a follow-up issue...

JKRhb commented Dec 7, 2023

View reviewed changes

packages/core/src/wot-impl.ts Outdated Show resolved Hide resolved

JKRhb force-pushed the explorer-directory branch 4 times, most recently from 2e31dcc to 783f4a0 Compare December 14, 2023 11:02

JKRhb added 2 commits December 14, 2023 12:06

feat(content-serdes): add application/ld+json to supported Content-Types

bfcf85c

This is required for fully supporting the `exploreDirectory` method. See https://www.w3.org/TR/wot-discovery/#exploration-directory-api-things-listing

feat(core): implement exploreDirectory method

77b916f

JKRhb force-pushed the explorer-directory branch from 783f4a0 to 77b916f Compare December 14, 2023 11:12

JKRhb marked this pull request as ready for review December 14, 2023 11:12

fixup! feat(core): implement exploreDirectory method

e9c4881

danielpeintner reviewed Dec 15, 2023

View reviewed changes

fixup! feat(core): implement exploreDirectory method

4a50219

danielpeintner reviewed Dec 15, 2023

View reviewed changes

danielpeintner approved these changes Dec 15, 2023

View reviewed changes

JKRhb added 5 commits December 15, 2023 17:24

fixup! feat(core): implement exploreDirectory method

455af97

test: add test for exploreDirectory method

9faa9d2

fixup! test: add test for exploreDirectory method

db19eae

fixup! feat(core): implement exploreDirectory method

7eddb7f

fixup! feat(core): implement exploreDirectory method

764d9fa

danielpeintner approved these changes Dec 19, 2023

View reviewed changes

relu91 reviewed Dec 20, 2023

View reviewed changes

fixup! feat(core): implement exploreDirectory method

0514567

JKRhb added 2 commits December 21, 2023 09:34

fixup! feat(core): implement exploreDirectory method

57affb8

fixup! test: add test for exploreDirectory method

951ca10

JKRhb force-pushed the explorer-directory branch from b292de9 to 951ca10 Compare December 21, 2023 10:12

JKRhb added 4 commits December 21, 2023 11:12

fixup! test: add test for exploreDirectory method

cddd356

fixup! feat(core): implement exploreDirectory method

99e102b

fixup! feat(core): implement exploreDirectory method

6f11544

refactor(core): use validation functions for requestThingDescription

568ebcc

JKRhb added 3 commits December 21, 2023 11:38

fixup! test: add test for exploreDirectory method

5e17c5f

fixup! test: add test for exploreDirectory method

a06e024

fixup! feat(core): implement exploreDirectory method

66104cf

JKRhb added 2 commits December 21, 2023 11:52

fixup! test: add test for exploreDirectory method

c8fc93f

fixup! feat(core): implement exploreDirectory method

7cc777f

relu91 approved these changes Dec 21, 2023

View reviewed changes

danielpeintner reviewed Dec 21, 2023

View reviewed changes

danielpeintner mentioned this pull request Dec 21, 2023

PR 1186 follow-ups w.r.t. validation #1206

Open

danielpeintner merged commit e05d67c into eclipse-thingweb:master Dec 21, 2023
12 checks passed

JKRhb deleted the explorer-directory branch December 21, 2023 15:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): implement `exploreDirectory` method #1186

feat(core): implement `exploreDirectory` method #1186

JKRhb commented Dec 7, 2023

codecov bot commented Dec 7, 2023 •

edited

Loading

JKRhb commented Dec 7, 2023

JKRhb commented Dec 14, 2023

danielpeintner commented Dec 15, 2023

danielpeintner left a comment

relu91 commented Dec 15, 2023

JKRhb commented Dec 15, 2023

danielpeintner Dec 15, 2023

JKRhb Dec 15, 2023

danielpeintner left a comment

danielpeintner left a comment

relu91 left a comment

danielpeintner commented Dec 20, 2023

JKRhb commented Dec 20, 2023

JKRhb commented Dec 21, 2023

JKRhb commented Dec 21, 2023 •

edited

Loading

relu91 left a comment

relu91 commented Dec 21, 2023

danielpeintner Dec 21, 2023

danielpeintner commented Dec 21, 2023 •

edited

Loading

		export function isThingDescription(input: unknown): input is WoT.ThingDescription {
		// TODO: Improve validation function

feat(core): implement exploreDirectory method #1186

feat(core): implement exploreDirectory method #1186

Conversation

JKRhb commented Dec 7, 2023

codecov bot commented Dec 7, 2023 • edited Loading

Codecov Report

JKRhb commented Dec 7, 2023

JKRhb commented Dec 14, 2023

danielpeintner commented Dec 15, 2023

danielpeintner left a comment

Choose a reason for hiding this comment

relu91 commented Dec 15, 2023

JKRhb commented Dec 15, 2023

danielpeintner Dec 15, 2023

Choose a reason for hiding this comment

JKRhb Dec 15, 2023

Choose a reason for hiding this comment

danielpeintner left a comment

Choose a reason for hiding this comment

danielpeintner left a comment

Choose a reason for hiding this comment

relu91 left a comment

Choose a reason for hiding this comment

danielpeintner commented Dec 20, 2023

JKRhb commented Dec 20, 2023

JKRhb commented Dec 21, 2023

JKRhb commented Dec 21, 2023 • edited Loading

relu91 left a comment

Choose a reason for hiding this comment

relu91 commented Dec 21, 2023

danielpeintner Dec 21, 2023

Choose a reason for hiding this comment

danielpeintner commented Dec 21, 2023 • edited Loading

feat(core): implement `exploreDirectory` method #1186

feat(core): implement `exploreDirectory` method #1186

codecov bot commented Dec 7, 2023 •

edited

Loading

JKRhb commented Dec 21, 2023 •

edited

Loading

danielpeintner commented Dec 21, 2023 •

edited

Loading