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

Discovery API - Alternatives #364

Closed
JKRhb opened this issue Jan 14, 2022 · 55 comments
Closed

Discovery API - Alternatives #364

JKRhb opened this issue Jan 14, 2022 · 55 comments
Labels
API-improvement Suggestions for changing the API discovery Relates to discovery and/or relates to joint work/discussions with the discovery task force propose closing Solutions exists and labelled as to be closed soon

Comments

@JKRhb
Copy link
Member

JKRhb commented Jan 14, 2022

When trying to implement the current discovery API I got the impression that its current design could be simplified by using a structure similar to the subscription/observation of events/properties. In particular, I had problems with implementing step two of the next() method:

If the active property is true, wait until the discovery results internal slot is not empty.

Correct me if I am wrong, but it seemed to me as if this would only be possible by continuously checking the results, using a function like setTimeout or within a while loop (are there any implementation examples?). As this didn't seem that optimal to me, I tried out a callback based approach instead (similar to the Subscription interface and the observeProperty/subscribeEvent methods) which worked quite well so far.

I therefore wanted to suggest changing the signature of the discover method (or its potential successors which are discussed in #222) to something like

ThingDiscovery discover(DiscoveryListener listener, optional ThingFilter thingFilter = null);

where the DiscoveryListener function signature could look like the following:

type DiscoveryListener = (thingDescription: ThingDescription) => void;

A minimal example could look like this:

async function handleDiscovery(thingDescription: ThingDescription) {
    const consumedThing = await wot.consume(thingDescription);
    await consumedThing.invokeAction("foo");
}


wot.discover(handleDiscovery, { method: "direct", url: "coap://example.org/td" );
// Calling stop() can probably be omitted when the "direct" method is being used.

Here, the discovery process is initialized using the direct method. When a Thing Description has been retrieved successfully, the handleDiscovery function is called which consumes the TD and interacts with the Thing. If I am not mistaken, a similar approach (using a class called Observable) was already part of the specification some time ago but I still wanted to open this issue to discuss it as an alternative to the current discover method.

I am looking forward to your feedback!

@JKRhb JKRhb changed the title Using a Subscription and callback for Discovery? Using a callback-based approach for Discovery? Jan 14, 2022
@JKRhb JKRhb added the discovery Relates to discovery and/or relates to joint work/discussions with the discovery task force label Jan 14, 2022
@danielpeintner
Copy link
Contributor

Scripting Call 2022-01-17

  • collect pros and cons
  • collect/write examples?
  • align with other API style, subscription mechanism
  • in case of same power: difficulty of implementing one or the other (e.g, blocking until new data is available)

@danielpeintner
Copy link
Contributor

I have been thinking about this proposal and I like it. It well aligns with what we have for Subscription.

w.r.t. "power" I think one can achieve the same, e.g.,

  • the discovery listener can stop() if the desired result has been found

@JKRhb
Copy link
Member Author

JKRhb commented Jan 20, 2022

@danielpeintner Thank you for the feedback!

w.r.t. "power" I think one can achieve the same, e.g.,

  • the discovery listener can stop() if the desired result has been found

I would think so, too :) The only aspect that is probably not covered at the moment is pagination, but maybe this could be handled by adding additional parameters to the DiscoveryListener type definition. I guess we need an example to figure out how this use case could be handled.

Also, I had two ideas which could probably be further improvements to the approach from above. The first one would be to add a stop() callback parameter to the DiscoveryListener type definition, so that you can easily stop the discovery process once a certain condition is fulfilled. In the example below, the discovery would be stopped once a TD with a "foo" action is found. Maybe this approach could also be used to handle pagination in the background -- another "page" of TDs is only fetched from a directory if the discovery hasn't been cancelled by the user based on their own decision criteria.

The other idea I had was to turn the start() method of the ThingDiscovery object into a restart() method which could be used to reinitiate a discovery process. A separate start() method, so I thought, is probably not needed as the discovery process should start once the ThingDiscovery is initialized. Let me know what you think about this :)

async function handleDiscovery(thingDescription: ThingDescription, stop: () => void) {
    if (thingDescription.actions.has("foo") {
        stop();
        const consumedThing = await wot.consume(thingDescription);
        await consumedThing.invokeAction("foo");
    }
}


const discovery = wot.discover(handleDiscovery, { method: "directoy", url: "coap://example.org/tdd" );
// ... some time passes, then the discovery process is started again
discovery.restart();
// Then the discovery process is cancelled "manually".
discovery.stop();

@danielpeintner
Copy link
Contributor

Some questions...

Also, I had two ideas which could probably be further improvements to the approach from above. The first one would be to add a stop() callback parameter to the DiscoveryListener type definition, so that you can easily stop the discovery process once a certain condition is fulfilled.

What is the difference of having stop() on ThingDiscovery ?
For subscriptions we have stop() also on Subscription.

The other idea I had was to turn the start() method of the ThingDiscovery object into a restart() method which could be used to reinitiate a discovery process.

The alternative would be to create yet another ThingDiscovery OR is your assumption that restart() does not report again the matches it found before being stopped?

@JKRhb
Copy link
Member Author

JKRhb commented Jan 20, 2022

What is the difference of having stop() on ThingDiscovery ?
For subscriptions we have stop() also on Subscription.

The additional stop() would mostly be for convenience as (if I am not mistaken) you would need a construct like this to access the discovery object from the handler function:

let discovery;
async function handleDiscovery(thingDescription: ThingDescription) {
    if (discovery != null) {
        discovery.stop();
    }
    ...
}

discovery = wot.discover(handleDiscovery, { ... );

This stop() callback would be the same as the stop() method, though. However, adding it as a callback would avoid the need for the null check and make stopping the process from the handler a bit more elegant.

The alternative would be to create yet another ThingDiscovery OR is your assumption that restart() does not report again the matches it found before being stopped?

That would also be an option :) I thought that restart() would behave exactly as the initial discovery. This could be useful if you don't store the discovered TDs but perform a single action on them and then throw them away, for example. Or if you a common discovery pattern that will be restarted frequently.

@JKRhb
Copy link
Member Author

JKRhb commented Jan 21, 2022

I've been thinking about the restart() question a bit more and now arrived at the conclusion that the start() already behaves like a restart. As you might want to instantiate a ThingDiscovery object "manually" I think keeping the name start() could therefore make more sense.

@zolkis
Copy link
Contributor

zolkis commented Jan 24, 2022

For API design here, let's also keep in mind browser event loop considerations. Probably known content, but anyway :).
What the heck is the event loop anyway? (youtube)
In the loop (youtube)

Edit. For instance, we should check every place we call callbacks in the spec, as they need to move to "queue a task for..." constructs for that.

@danielpeintner
Copy link
Contributor

relates to #222

@danielpeintner
Copy link
Contributor

danielpeintner commented Jan 31, 2022

Scripting Call 2022-01-31

  • @JKRhb creates PR (or updates the issue) with WebIDL changes
  • consecutively we discuss the updates

@zolkis
Copy link
Contributor

zolkis commented Jan 31, 2022

A possible variant for a callback based discovery API uses the Subscription object, that already has an active flag and a stop() method that can also accept interaction options.
However, the algorithms should have steps on how exactly the Subscription should be used.

callback DiscoveryHandler = undefined(ThingDescription);

partial namespace WOT {
  Subscription discover(DiscoveryHandler handler, optional ThingFilter filter = null);
};

typedef DOMString DiscoveryMethod;

dictionary ThingFilter {
  DiscoveryMethod method = "directory";
  USVString? url;
  object? fragment;
 };

@zolkis
Copy link
Contributor

zolkis commented Jan 31, 2022

A possible API with direct functions.

callback DiscoveryHandler = undefined(ThingDescription);

partial namespace WOT {
[SecureContext, Exposed=(Window,Worker)]
interface WotDiscovery {
    Promise<ThingDescription> direct(USVString url);
    Promise<Subscription> directory(DiscoveryHandler handler, USVString url);
};

With examples like:

let discovery = new WotDiscovery();
let td = await discovery.direct("https://mythings.my/thing2");
let subs = await discovery.directory(handler, "https://directory.mythings.my/");
...
if (subs.active) {
   await subs.stop();
}

That allows for a small variation.

callback DiscoveryHandler = undefined(ThingDescription);

partial namespace WOT {
[SecureContext, Exposed=(Window,Worker)]
interface WotDirectDiscovery {
    Promise<ThingDescription> start(USVString url);
};

[SecureContext, Exposed=(Window,Worker)]
interface WotDirectoryDiscovery extends Subscription {
    Promise<undefined> start(DiscoveryHandler handler, USVString url);
};

The example becomes:

let discovery = new WotDirectDiscovery();
let td = await discovery.start("https://mythings.my/thing2");

let discovery = new WotDirectoryDiscovery();
await discovery.start(handler, "https://directory.mythings.my/");
...
if (discovery.active) {
    await discovery.stop();
}

Edit: changed the direct discovery to a promise, as we don't need a handler there, and it also manages the errors.
Note that the directory method is missing ways to handle errors during discovery (however, I don't see necessary to expose those in discovery). We could pass an error callback as well, if needed.

@relu91 PTAL

@zolkis
Copy link
Contributor

zolkis commented Feb 1, 2022

Yet another variation. I like that direct() returns a promise that resolves with a TD.
If somehow we could expose also directory discovery with that pattern, that would also solve the follow-links problem.
That means it could return an iterable , which brings back the next() method we wanted to get rid of :).

So it might be that the current discovery API is still the most idiomatic and flexible:

  • it implements an iterable, i.e. a standard and same interface for all discovery methods;
  • therefore it has flow control (script calling next()), unlike with callbacks or events (though this is not really a problem because of how JS engines work)
  • it handles errors both before and during discovery (the callback alternative would need to pass 2 callbacks); however, the way it is specified now, is the same as the promise version of directory discovery (only the last error available that was so severe as to stop discovery).
  • extensible with more discovery methods without an API/ABI break (the others can do the same).

Now we have a lot of options to discuss :).

@relu91
Copy link
Member

relu91 commented Feb 3, 2022

Yet another variation. I like that direct() returns a promise that resolves with a TD. If somehow we could expose also directory discovery with that pattern, that would also solve the follow-links problem. That means it could return an iterable , which brings back the next() method we wanted to get rid of :).

So it might be that the current discovery API is still the most idiomatic and flexible:

  • it implements an iterable, i.e. a standard and same interface for all discovery methods;
  • therefore it has flow control (script calling next()), unlike with callbacks or events (though this is not really a problem because of how JS engines work)
  • it handles errors both before and during discovery (the callback alternative would need to pass 2 callbacks); however, the way it is specified now, is the same as the promise version of directory discovery (only the last error available that was so severe as to stop discovery).
  • extensible with more discovery methods without an API/ABI break (the others can do the same).

Now we have a lot of options to discuss :).

I agree that the current API can cover the points above. Still, it is an overkill for the simplest use-case:

  • Return the list of TDs at this location

I agree there's still a lot to discuss, taking also in mind that the Discovery specification is still ongoing (even though close to the feature freeze).

@zolkis
Copy link
Contributor

zolkis commented Feb 3, 2022

Return the list of TDs at this location

Handling a list on the client side is best done with iterators, since this can encapsulate buffering, unlike when we expose arrays of TDs to the scripts. Providing a callback (two, actually) is another pattern, but splitting into direct() and directory() ends up with different function signatures, whereas with the current design the same function can handle both, based on a string argument "direct" or "directory".

We could make Example 9 and Example 10 with all alternatives as see which works best.

@JKRhb
Copy link
Member Author

JKRhb commented Feb 7, 2022

I guess such an iterator would/should be an async one, right? It seems to me as if an Async Iterable could solve the blocking issue I mentioned above very elegantly.

@zolkis
Copy link
Contributor

zolkis commented Feb 7, 2022

Yes, we could use an async iterator on the discovery object, they are supported in ECMAScript.

@JKRhb
Copy link
Member Author

JKRhb commented Feb 10, 2022

Yes, we could use an async iterator on the discovery object, they are supported in ECMAScript.

Okay, great :) Do I see it correctly that we could then handle discovery like so?

const discovery = WoT.discover({ method: "directory", url: "http://directory.example.org" });

for await (const thingDescription of discovery) { // Note: Has to be within an async function
  console.log("Found Thing Description for " + thingDescription.title);
}

If so, I think this would be a very nice solution.

@danielpeintner
Copy link
Contributor

Today's Scripting call

@zolkis
Copy link
Contributor

zolkis commented Feb 21, 2022

Okay, great :) Do I see it correctly that we could then handle discovery like so?

const discovery = WoT.discover({ method: "directory", url: "http://directory.example.org" });

for await (const thingDescription of discovery) { // Note: Has to be within an async function
  console.log("Found Thing Description for " + thingDescription.title);
}

Yes, the implementations need to handle Symbol.asyncIterator / an async generator.
We should be also able to say

if (discovery.active) {
    await discovery.stop();
}

@zolkis
Copy link
Contributor

zolkis commented May 23, 2022

@JKRhb are you planning to experiment with iterators in the implementation?
If there is any feedback to the API, please make them here.

@JKRhb
Copy link
Member Author

JKRhb commented May 25, 2022

@JKRhb are you planning to experiment with iterators in the implementation? If there is any feedback to the API, please make them here.

Sure, I will try to come with an example based on node-wot until the next meeting which could serve as a basis for the new API.

@JKRhb
Copy link
Member Author

JKRhb commented May 25, 2022

I already tried to experiment with Iterators a bit, and it seems to if an AsyncGenerator could provide a very elegant solution to the problem (using a class that implements AsyncIterable caused some problems for me, as you cannot access class fields from the AsyncIterator, as it seems). A first example can be seen below. Maybe this could work for the new API, provided that it can be expressed in WebIDL?

You can experiment with the code yourselves in the Typescript Playground under this link.

interface ThingDescription {
  title: string
}

class ThingDiscovery {
  active: boolean = true;

  async *discover(uri: string): AsyncGenerator<ThingDescription> {
    while (this.active) {
        const response = await fetch(uri);
        const parsedTd = await response.json();
        yield new Promise<ThingDescription>(resolve => resolve(parsedTd));
    }
}

  public stop() {
    this.active = false;
  }
}

// Just a random URI pointing to a TD
const uri = "https://raw.githubusercontent.com/w3c/wot-testing/main/events/2022.03.Online/TD/hitachi-esp-idf/TDs/hitachi-acc-air.td.jsonld";

async function startDiscovery() {
  let thingDiscovery = new ThingDiscovery();
  let counter = 0;
  for await (let thingDescription of thingDiscovery.discover(uri)) {
    if (counter === 3) {
      thingDiscovery.stop();
      break;
    }
    console.log(`Fetched a Thing Description with title ${thingDescription.title}`);
    counter++;
  }
}

startDiscovery();

@JKRhb
Copy link
Member Author

JKRhb commented May 25, 2022

Noticing that the class semantics are a bit off in the example above, I tried again to use an AsyncIterator and this time it worked :) You can also try out in this example in the TS playground under this link.

interface ThingDescription {
  title: string
}

class WoT {

  discover (uri: string): ThingDiscovery {
    return new ThingDiscovery(uri);
  }

}

class ThingDiscovery implements AsyncIterable<ThingDescription> {

  active: boolean = true;
  private uri: string;

  constructor(uri: string) {
    this.uri = uri;
  }

  async* [Symbol.asyncIterator](): AsyncIterator<ThingDescription> {
      while (this.active) {
        const response = await fetch(this.uri);
        const parsedTd = await response.json();
        yield new Promise<ThingDescription>(resolve => resolve(parsedTd));
    } 
  }

  public stop() {
    this.active = false;
  }
}

// Just a random URI pointing to a TD
const uri = "https://raw.githubusercontent.com/w3c/wot-testing/main/events/2022.03.Online/TD/hitachi-esp-idf/TDs/hitachi-acc-air.td.jsonld";

async function startDiscovery() {
  const wot = new WoT();
  let thingDiscovery = wot.discover(uri);
  let counter = 0;
  for await (let thingDescription of thingDiscovery) {
    if (counter === 3) {
      thingDiscovery.stop();
      break;
    }
    console.log(`Fetched a Thing Description with title ${thingDescription.title}`);
    counter++;
  }
}

startDiscovery();

@JKRhb
Copy link
Member Author

JKRhb commented May 25, 2022

Experimenting a bit more, I noticed that with the latest approach you can also access the iterator itself and call next() like so:

const iterator = thingDiscovery[Symbol.asyncIterator]();
const thingDescription = (await iterator.next()).value;
console.log(thingDescription);

@JKRhb
Copy link
Member Author

JKRhb commented May 30, 2022

I created a new example which both implements a next() method and adds the start() method back in. I also cleaned up the example in general, making it clearer that only one TD is actually discovered by the direct method (at least in this case).

Edit: You can also try out the example directly in the Typescript playground using this link.

Third Discovery Example in Typescript
interface ThingDescription {
  title: string
}

class WoT {

  discover (uri: string): ThingDiscovery {
    const thingDiscovery = new ThingDiscovery(uri);
    thingDiscovery.start();
    return thingDiscovery;
  }

}

class ThingDiscovery implements AsyncIterable<ThingDescription> {

  active: boolean = false;
  private uri: string;

  constructor(uri: string) {
    this.uri = uri;
  }

  public async next()  {
    return await this[Symbol.asyncIterator]().next();
  }

  async* [Symbol.asyncIterator](): AsyncIterator<ThingDescription> {
    if (!this.active) {
      return;
    }

    const response = await fetch(this.uri);
    const parsedTd = await response.json();
    yield new Promise<ThingDescription>(resolve => resolve(parsedTd));
  }

  public stop() {
    this.active = false;
  }

  public start() {
    this.active = true;
  }
}

// Just a random URI pointing to a TD
const uri = "https://raw.githubusercontent.com/w3c/wot-testing/main/events/2022.03.Online/TD/hitachi-esp-idf/TDs/hitachi-acc-air.td.jsonld";

async function startDiscovery() {
  const wot = new WoT();
  const thingDiscovery = wot.discover(uri);
  for await (let thingDescription of thingDiscovery) {
    console.log(`Fetched a Thing Description with title ${thingDescription.title}`);
  }
  // Alternative:
  console.log(await thingDiscovery.next());

  // Stop the discovery process
  thingDiscovery.stop();

  // Does not yield results, as the discovery process has been stopped.
  for await (let thingDescription of thingDiscovery) {
    console.log(`This will not be displayed`);
  }

  // Returns {"value": undefined, "done": true}
  console.log(await thingDiscovery.next());
}

startDiscovery();

@zolkis
Copy link
Contributor

zolkis commented May 30, 2022

Thanks Jan. Minor nit: according to the spec, start() is invoked from discover(), not from the constructor.

@danielpeintner
Copy link
Contributor

After the discussion we had in today's call I personally see the following interfaces as a possible way forward...

partial namespace WOT {
  ThingDiscovery discover();
};

interface ThingDiscovery {
  // return TD directly
  Promise<ThingDescription> direct(string url);
  // return TDs via Iterator
  DiscoveryIterator directory(string url, ...);
  DiscoveryIterator any(string url, ...);
  // more ?
};

interface DiscoveryIterator {
  // decide which properties we actually need
  readonly attribute boolean active;
  readonly attribute boolean done;
  readonly attribute Error? error;
  readonly attribute ThingFilter? filter;
  // actual iterator methods
  undefined start();
  undefined stop();
  Promise<ThingDescription> next();
  async iterable<ThingDescription>;
};

@zolkis
Copy link
Contributor

zolkis commented Nov 7, 2022

A few comments,

  • I am not sure if DiscoveryIterator is the best name, but why not.
  • We should use USVString for URLs.
  • We need to be able to pass discovery options to methods. Maybe different ones for different methods.
  • start() is unclear now
  • we don't need next()
  • any() is not a good name
  • For async methods, we should be able to pass and AbortSignal among the options, if we choose to respect the design doc. But I think stop() should be fine in this limited context.
  • Not sure if we want to keep the factory method, or should we just have a constructor.
  • (EDIT) I realized we need to move stop() out of the iterable, and likely the rest, too.

The corrected Web IDL for @danielpeintner 's suggestion:

partial namespace WOT {
  ThingDiscovery discover();   // we don't really need this, do we?
};

interface ThingDiscovery {
  constructor();
  Promise<ThingDescription> direct(USVString url, optional ThingFilter options);
  DiscoveryResults directory(USVString url, optional ThingFilter options);
  DiscoveryResults queryAll(optional ThingFilter options);
  undefined stop(); 

  // decide which properties we actually need
  readonly attribute boolean active;  // discovery has started
  readonly attribute boolean done;  // mapped to iterators' `done`
  readonly attribute Error? error;
  readonly attribute ThingFilter? filter;
};

interface DiscoveryResults {
  async iterable<ThingDescription>;
};

@zolkis
Copy link
Contributor

zolkis commented Nov 7, 2022

(Edited).
I wonder if we could go further. Should we map out direct discovery as a method of its own, and use the term discovery for the rest? Instead of the former name fetch(), we could use less overlapping names like requestThingDescription() or readThingDescription() .
Also, we could have a constructor for iterator-style discovery, and an async method for direct fetch.
The Web IDL that implements this:

partial namespace WOT {
  Promise<ThingDescription> requestThingDescription(USVString url, optional ThingFilter options);
};

interface ThingDiscovery {
  constructor();

  readonly attribute boolean active;
  readonly attribute boolean done;
  readonly attribute Error? error;  // protocol errors 
  readonly attribute ThingFilter? filter;
 
  // runtime errors (not protocol errors) are handled by Promise
  Promise<undefined> queryDirectory(USVString url, optional ThingFilter options);
  Promise<undefined> queryAll(optional ThingFilter options);
  undefined stop();
  async iterable<ThingDescription>;
};

Note that the query...() methods are a specific kind of start() method. Either one makes active true, and the algorithms will make the other calls fail if a discovery is active. In other words, it's a syntactic sugar to what we have now, essentially a single start() method plus a parameter for the discovery method. I don't see major difference there.

An example of using this "combined" (control + iterator) interface, in current and new versions.

let discovery = new ThingDiscovery({ method: "direct" });
for await (let td of discovery) {
  console.log("Found Thing Description for " + td.title);
}
let discovery = new ThingDiscovery();
discovery.queryAll().then( () => {
  for await (let td of discovery) {
    console.log("Found Thing Description for " + td.title);
  }
  // Check if iterator stopped since value not available because of an error.
  if (discovery.error) {  
      console.log("There was a protocol error": + discovery.error.message);
  }
}).catch(error) {
  console.log("Could not start discovery: " error.message);  // maybe security error
}

@zolkis zolkis pinned this issue Nov 8, 2022
@zolkis
Copy link
Contributor

zolkis commented Nov 9, 2022

Note that the proposal here (Daniel's modified) and here (my last) are almost the same, except

  • the latter has different method names,
  • the latter experiments with returning a promise for starting discovery (algorithm steps will help clarify which approach is better)
  • the latter implements async iterable directly.

We can discuss these differences separately, but the starting point is the first proposal (modified from Daniel's). I think it's a good start. It would also allow something like the following:

try {
  for await (let td of wot.discover().queryAll()) {
    console.log("Found Thing Description for " + td.title);
  }
} catch(error) {
  console.log("There was an error": + error.message);
}

Which is pretty nice, minimal, and feasible (if we define properly how to throw).

@relu91 @JKRhb your turn. :)

@relu91
Copy link
Member

relu91 commented Nov 14, 2022

I read through this issue thread and also read the Discovery spec once again. I think we are getting closer to a consensus.

Some doubts that I still have:

  1. ThingDiscovery contains both methods and discovery process status. This may complicate the algorithms because they need to check the weather there is a process already ongoing or not.
  2. There might be an use case where the WoT app developer is unsure whether a given URL is for a TDD or a single TD. With this APIs he has to call requestThingDescription, check weather it is a TDD, and then call queryDirectory. It would be nice to have a method that cover also this corner case. Maybe we can refactor this queryDirectory to query and it will act accordingly.

To solve 1. I would:

partial namespace WOT {
  Promise<ThingDescription> requestThingDescription(USVString url, optional ThingFilter options);
};

interface ThingDiscovery {
  constructor();
  // runtime errors (not protocol errors) are handled by Promise
  Promise<ThingDiscoveryProcess> queryDirectory(USVString url, optional ThingFilter options);
  Promise<ThingDiscoveryProcess> queryAll(optional ThingFilter options);
  
};

interface ThingDiscoveryProcess {
  readonly attribute boolean active;
  readonly attribute boolean done;
  readonly attribute Error? error;  // protocol errors 
  readonly attribute ThingFilter? filter;

  undefined stop();
  async iterable<ThingDescription>;

}
 

@zolkis
Copy link
Contributor

zolkis commented Nov 14, 2022

I like the latest approach. We need to think about how future use cases would change the API: local, bluetooth/nfc, multicast etc. I think they could be covered by the queryAll() method, or we can define separate methods as well (I prefer the latter, because the dedicated algorithms and separate problem space).

@danielpeintner
Copy link
Contributor

Scripting Call 2022-11-14

  • Use-cases
    • List of urls (vs. single url)
    • removing duplicates (coming from discovery spec)
    • manual TDD discovery (get TD & detect TDD) vs automatically dealing by providing an url
    • automatic discovery dealing with ThingLinks

TODO / questions:

  • update the proposal?
  • @ashimura ask developers for priority of the afore mentioned use-cases
  • different ThingFilter options for queryDirectory and queryAll
  • What should be under WoT namespace
  • ...

@zolkis
Copy link
Contributor

zolkis commented Nov 14, 2022

Just writing down what @relu91 proposed (with point 2)
Note that conflicts with the requirement from @danielpeintner that we should not be forced to use iterator when fetching a single TD.
Now we can't have both, so we need to pick a choice :).
IMHO the more developer friendly option is what's below (no real problem handling an iterator for a single TD).

interface ThingDiscovery {
  constructor();
  // runtime errors (not protocol errors) are handled by Promise
  // query() is manual (script controls the traversal), 
  //    handles both fetching a single TD, and a TDD
  Promise<ThingDiscoveryProcess> query(USVString url, optional QueryFilter options);
 
  // queryAll() leaves all complexity to the implementation, 
  //    and MAY specify URL filters as well
  Promise<ThingDiscoveryProcess> queryAll(optional ExtendedQueryFilter options);
};

interface ThingDiscoveryProcess {
  readonly attribute boolean active;
  readonly attribute boolean done;
  readonly attribute Error? error;  // protocol errors 
  // readonly attribute ThingFilter? filter; // no longer needed

  undefined stop();
  async iterable<ThingDescription>;
};

An alternative would be to expose both query() and queryAll() directly from the WoT namespace, as discover() and discoverAll(). That would address the concern of differing from the style of produce() and consume().
If we have a separate method for reading a TD, that could as well be directly on the WoT object as requestThingDescription().

@danielpeintner
Copy link
Contributor

I think we are converging :-)

I was asking myself "what" should happen if query(...) gets called with an url of directory. Shall it return the ThingDescription of the Directory or (as in your case) the actual thing TDs behind the directory. I tend to agree with your thinking.

Note: I think readonly attribute ThingFilter? filter; in ThingDiscoveryProcess might no longer be necessary since it may be QueryFilter or ExtendedQueryFilter now. I would say that this information might not be needed anyway... people could argue we need to store the entry url as well.

discoverAll(). That would address the concern of differing from the style of produce() and consume().
If we have a separate method for reading a TD, that could as well be directly on the WoT object as requestThingDescription().

👍

@zolkis
Copy link
Contributor

zolkis commented Nov 14, 2022

Note: I think readonly attribute ThingFilter? filter; in ThingDiscoveryProcess might no longer be necessary

That is right. Removed.

@zolkis
Copy link
Contributor

zolkis commented Nov 14, 2022

To record the current stage of discussion:

partial namespace WOT {
  // get a single TD, or fail
  Promise<ThingDescription> requestThingDescription(USVString url);

  // Manual traversal of TDD, but also handles single TD at URL
  Promise<ThingDiscoveryProcess> discover(USVString url, optional ThingFilter options);
 
  // discoverAll() leaves all control and complexity to the implementation, 
  //    and MAY specify URL filters as well
  Promise<ThingDiscoveryProcess> discoverAll(optional ExtendedThingFilter options);
};

interface ThingDiscoveryProcess {
  readonly attribute boolean active;
  readonly attribute boolean done;
  readonly attribute Error? error;  // protocol errors 

  undefined stop();
  async iterable<ThingDescription>;
};

I think we can have a separate method to "get a single TD or else fail" - I have removed the ThingFilter options from it (not really needed, since one or zero TD is expected anyway).

The discover() method here is primarily for getting TDs from a directory, but would also handle the case when there is a single TD at the given URL. This would accept filters.

Obviously discoverAll() will be the trickiest to specify consistently, we'll see what modifications will it eventually need when writing the algorithm/implementation.

@zolkis
Copy link
Contributor

zolkis commented Nov 16, 2022

The IDL above doesn't look much different from the current do-it-all discover(options) method, where options contains a URL. :). Maybe some renaming would make sense, perhaps discoverFrom(url, options) - since using the name "directory" is not really good, once we also wanted it to cover eventual single TD. Speaking about that, we should check the use cases: in a given solution, for which a script is being written, isn't it always known that a given URL is meant to be a directory in given solution? Or we cannot make the assumption the script developer has some special knowledge about the URLs?

I can imagine the following use cases:

  • request a TD from a random URL (check if it's a Thing). That can return nothing, or a TD that can be a Thing or a Directory. The script will have to figure out alone which.
  • request discovery at (from) a given URL, assuming it's a TD (and if it's not a TD, get an error right away).
  • request discovery from a given URL, without assumptions (returns an error, or a single TD, or a bunch of TDs), and possible filters -- the discover() method above
  • request generic discovery, given some filters -- the discoverAll() method above.
    Which use cases should we support? Any others?

@relu91
Copy link
Member

relu91 commented Nov 21, 2022

Thank you for writing down the use cases. They fit more or less the ones that I had in mind in the last call. To use a more Discovery spec oriented terminology:

  • request generic discovery, given some filters -- the discoverAll() method above. This means letting the runtime support a bunch of known introduction mechanisms and automatically explore the resulting set of discovered URLs.

Also, I think use case 2 is supported by 1, right?

Basically in mind, we should support:

  1. Fetch a TD or throw an error (it can be any URL accepted by the protocol binding templates spec)
  2. Discovery of URL with filtering ( we could be merciful and if the TD in the URL is not a TDD we can still try to apply the filters but it will result in some implementation costs)
  3. Discover what you can.

Those cover the plausible scenarios in WoT application development:

  1. I know exactly which TD I want
  2. URL is issued by a third party and I have no means to verify if he is pointing me to a TDD or a TD
  3. I want to adapt to the deployment context and try to fetch what the deployer allowed me to do.

@zolkis
Copy link
Contributor

zolkis commented Nov 21, 2022

I know exactly which TD I want
URL is issued by a third party and I have no means to verify if he is pointing me to a TDD or a TD

These 2 could be supported by the requestThingDescription() method.

I want to adapt to the deployment context and try to fetch what the deployer allowed me to do.

That is the discoverAll().

What I am wondering about is the semantics of discover() should include only directories (error if not a TDD), or allow fetching a single TD as well (contained in the list, not the TD of the TDD itself, which could be obtained by the requestThingDescription().

@zolkis
Copy link
Contributor

zolkis commented Nov 21, 2022

The Web IDL discussed in the call.

partial namespace WOT {
  // get a single TD, or fail
  Promise<ThingDescription> requestThingDescription(USVString url);

  // Manual or automatic traversal of TDD
  Promise<ThingDiscoveryProcess> exploreDirectory(USVString url, optional ThingFilter options);
 
  // discover() leaves all control and complexity to the implementation, 
  //    and MAY specify URL filters as well
  Promise<ThingDiscoveryProcess> discover(optional ExtendedThingFilter options);
};

interface ThingDiscoveryProcess {
  readonly attribute boolean active;
  readonly attribute boolean done;
  readonly attribute Error? error;  // protocol errors 

  undefined stop();
  async iterable<ThingDescription>;
};

dictionary ThingFilter {
  // USVString? query;
  object? fragment;
};

dictionary ExtendedThingFilter extends ThingFilter {
  USVString? url;
};

@danielpeintner
Copy link
Contributor

Scripting Call 2022-11-21

  • @relu91 @JKRhb will work on a first PR of the above API
  • others will review

@zolkis
Copy link
Contributor

zolkis commented Nov 22, 2022

I am thinking to add a filter option whether to (recursively) follow links (of other TDDs) in a TDD and how deep, or stay at the top level.

dictionary ThingFilter {
  // USVString? query;
  object? fragment;
  unsigned short traversalDepth = 1;  // follow directory links up to this depth, allowed 1-9?
  // or have a different name and functionality?
  // unsigned short? followLinks = 0;  // extra depth to follow TD links, sensible allowed range: 0-9?
};

Anyway, this will be done later.

@zolkis
Copy link
Contributor

zolkis commented Nov 28, 2022

In the call on 28.11.2022 we discussed how to expose local Things: via a filter, or via a separate method.
It seems local discovery involves an introduction phase and exploration phase, therefore a separate method and algorithm would be better suited (later).

@zolkis zolkis added the API-improvement Suggestions for changing the API label Jan 23, 2023
@JKRhb
Copy link
Member Author

JKRhb commented May 19, 2023

Hi everyone :) I was dealing with implementing directory discovery in dart_wot lately and I was wondering a bit about how exactly the process should work here:

  1. Is the directory URL supposed to be the directory's Thing Description? Or is it supposed to point to a resource related to the things property of the directory?
  2. In the case that the URL is supposed to point to the TDD's Thing Description, should the implementation first consume the TD and then read the things property? Or is this implementation-specific?

Sorry if the answer to these questions should be obvious, unfortunately, I haven't really been able to follow the discussions in the TF in detail for the last couple of months :/ In any case: Thank you! :)

@JKRhb
Copy link
Member Author

JKRhb commented Dec 11, 2023

I think we could consider closing this issue (since we now have a new API in place) and maybe create a new issue from my comment above (#364 (comment)) that deals with describing the algorithm used for the exploreDirectory method more explicitly.

@JKRhb JKRhb added the propose closing Solutions exists and labelled as to be closed soon label Dec 11, 2023
@zolkis
Copy link
Contributor

zolkis commented Dec 11, 2023

Do we consider this addressed by #441 ? If yes, we can close and continue discussing the new points in a new issue.

@relu91
Copy link
Member

relu91 commented Dec 11, 2023

Call 11 Dec

  • @JKRhb we should close this and create a new issue.

@relu91 relu91 closed this as completed Dec 11, 2023
@relu91 relu91 unpinned this issue Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API-improvement Suggestions for changing the API discovery Relates to discovery and/or relates to joint work/discussions with the discovery task force propose closing Solutions exists and labelled as to be closed soon
Projects
None yet
Development

No branches or pull requests

4 participants