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.

Sign up for GitHub

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

Jump to bottom

W3C compliance - Use forms instead of links in Properties, Actions, and Events #2811

Merged
merged 3 commits into from
Oct 13, 2021
Merged

W3C compliance - Use forms instead of links in Properties, Actions, and Events #2811

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
026936a
Use forms instead of links in Properties, Actions, and Events
relu91 Mar 16, 2021
4e8e206
Improve form href selection in UI
relu91 Oct 1, 2021
df8c281
Use base when selecting forms in frontend
relu91 Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/models/thing.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,8 @@ export default class Thing extends EventEmitter {

if (property.forms) {
property.forms = property.forms.map((form) => {
// TODO: WebThingsIO non-standard keyword
// TODO: webthings proprietary field;
// See https://github.com/WebThingsIO/gateway/issues/2832
if (form.proxy) {
delete form.proxy;
form.href = `${Constants.PROXY_PATH}/${encodeURIComponent(this.id)}${form.href}`;
Expand Down Expand Up @@ -768,7 +769,8 @@ export default class Thing extends EventEmitter {

if (event.forms) {
event.forms = event.forms.map((form) => {
// TODO: WebThingsIO non-standard keyword
// TODO: webthings proprietary field;
// See https://github.com/WebThingsIO/gateway/issues/2832
if (form.proxy) {
delete form.proxy;
form.href = `${Constants.PROXY_PATH}/${encodeURIComponent(this.id)}${form.href}`;
Expand Down
3 changes: 2 additions & 1 deletion static/js/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ module.exports.ThingFormat = {
};

module.exports.WoTOperation = {
READ_PROPERTY: 'readyproperty',
READ_PROPERTY: 'readproperty',
WRITE_PROPERTY: 'writeproperty',
INVOKE_ACTION: 'invokeaction',
READ_ALL_PROPERTIES: 'readallproperties',
SUBSCRIBE_ALL_EVENTS: 'subscribeallevents',
Expand Down
5 changes: 3 additions & 2 deletions static/js/models/thing-model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const API = require('../api').default;
const App = require('../app');
const Model = require('./model');
const Constants = require('../constants');
const Utils = require('../utils');

class ThingModel extends Model {
constructor(description, ws) {
Expand Down Expand Up @@ -194,7 +195,7 @@ class ThingModel extends Model {

const property = this.propertyDescriptions[name];

const href = property.forms[0].href;
const href = Utils.selectFormHref(property.forms, Constants.WoTOperation.READ_PROPERTY);

return API.putJson(href, value)
.then((json) => {
Expand All @@ -216,7 +217,7 @@ class ThingModel extends Model {
if (typeof this.propertiesHref === 'undefined') {
const urls = Object.values(this.propertyDescriptions).map((v) => {
if (v.forms) {
return v.forms[0].href;
return Utils.selectFormHref(v.forms, Constants.WoTOperation.WRITE_PROPERTY);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for extensibility it might be right to look for writeproperty operation here even if now it is just reduntat.

}
});
const requests = urls.map((u) => API.getJson(u));
Expand Down
9 changes: 7 additions & 2 deletions static/js/rules/PropertySelect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ const fluent = require('../fluent');
const RuleUtils = require('./RuleUtils');
const Units = require('../units');
const Utils = require('../utils');
const Constants = require('../constants');

function propertyEqual(a, b) {
if (!a && !b) {
Expand Down Expand Up @@ -373,8 +374,12 @@ class PropertySelect {
continue;
}

property.id = RuleUtils.extractProperty(forms[0].href);
property.thing = RuleUtils.extractThing(forms[0].href);
property.id = RuleUtils.extractProperty(
Utils.selectFormHref(forms, Constants.WoTOperation.READ_PROPERTY)
);
property.thing = RuleUtils.extractThing(
Utils.selectFormHref(forms, Constants.WoTOperation.READ_PROPERTY)
);
delete property.forms;

const name = property.title || Utils.capitalize(property.name);
Expand Down
16 changes: 2 additions & 14 deletions static/js/schema-impl/capability/thing.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,7 @@ class Thing {
}
}

let href;
for (const form of property.forms) {
if (!form.op || form.op === Constants.WoTOperation.READ_PROPERTY) {
href = form.href;
break;
}
}
const href = Utils.selectFormHref(property.forms, Constants.WoTOperation.READ_PROPERTY);

if (!href) {
continue;
Expand Down Expand Up @@ -300,13 +294,7 @@ class Thing {
for (const name in description.actions) {
const action = description.actions[name];

let href;
for (const form of description.actions[name].forms) {
if (!form.op || form.op === Constants.WoTOperation.INVOKE_ACTION) {
href = form.href;
break;
}
}
const href = Utils.selectFormHref(action.forms, Constants.WoTOperation.INVOKE_ACTION);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency, I always used selectFormHref when looking for the gateway form.


if (!href) {
continue;
Expand Down
35 changes: 35 additions & 0 deletions static/js/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
*/

import * as Fluent from './fluent';
import Constants from './constants';

/**
* @param {String} str
Expand Down Expand Up @@ -264,3 +265,37 @@ export function adjustInputValue(value: number, schema: Record<string, unknown>)

return value;
}

type WoTOperation = keyof typeof Constants.WoTOperation;
type WoTFormOperation = WoTOperation | WoTOperation[] | undefined;
/**
* Finds the gateway api endpoint for a specific operation.
* It uses the assumption that gateway enpoints are pushed to
* the form array as the last element.
*
* @param {Array} forms The list of forms.
* @param {string} operation
* @returns {string | undefined} the href of the select form or undefined if not found.
*/
export function selectFormHref(
forms: Array<{ href: string; op: WoTFormOperation }>,
operation: WoTOperation
): string | undefined {
return [...forms].reverse().find((selectedForm) => {
try {
const { protocol } = new URL(selectedForm.href);
return (
protocol === 'http:' &&
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be https:

(!selectedForm.op || selectedForm.op === operation || selectedForm.op?.includes(operation))
);
} catch (error) {
if (error instanceof TypeError) {
// URL is relative or not well formatted
Copy link
Member

@benfrancis benfrancis Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm right in saying that all the form hrefs added by the gateway are just paths rather than absolute URLs (e.g. /things/foo/properties/on). That means this logic would always be triggered and the protocol check would never be applied.

Rather than just ignore the protocol if not an absolute URL, I think if you're going to check the protocol what you need to do here is resolve the href against the base member of the Thing Description first.

This is probably strictly not necessary since the last form entry (added by the gateway) will always be HTTP, so we could just remove the protocol check if you prefer.

return (
!selectedForm.op || selectedForm.op === operation || selectedForm.op?.includes(operation)
);
}
throw error;
}
})?.href;
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file the right place to introduce this utility function?

I use the spread operator and the reverse function, although it might be not so efficient for a large forms array I think it is more expressive than a for loop that searches the form backward.

About the selection logic, tell me if you find it satisfactory. I choose to check also for protocol, maybe is an overkill? wot-adapter may publish TDs with other protocols in forms..

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wanted to provide unit tests for this particular function, but it is not easy to test the frontend code so I gave up. Feel free to suggest how to properly test this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file the right place to introduce this utility function?

If it was only needed inside thing-model.js I would have put it there since this logic is quite specific to parsing a Thing Description, but since it's needed in multiple files I think it's reasonable to put it in this central utilities file.

I use the spread operator and the reverse function, although it might be not so efficient for a large forms array I think it is more expressive than a for loop that searches the form backward.
About the selection logic, tell me if you find it satisfactory. I choose to check also for protocol, maybe is an overkill? wot-adapter may publish TDs with other protocols in forms..

I think what I would have done is to just traverse the array forwards looking for matches and pick the last result, since there could theoretically be multiple matches, but I think your approach has the same effect. As far as I know the Thing Description specification doesn't specify what to do if there are multiple forms for the same operation using the same protocol.

I agree that in theory we should check the protocol, but if you're already making the assumption that the last matching form is an endpoint added by the gateway, then you could possibly also assume that it will be using http/https since that's all the gateway currently exposes.

11 changes: 4 additions & 7 deletions static/js/views/things.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,13 +215,10 @@ const ThingsScreen = {
return;
}

let href;
for (const form of description.actions[actionName].forms) {
if (!form.op || form.op === Constants.WoTOperation.INVOKE_ACTION) {
href = form.href;
break;
}
}
const href = Utils.selectFormHref(
description.actions[actionName],
Constants.WoTOperation.INVOKE_ACTION
);

const icon = Icons.capabilityToIcon(description.selectedCapability);
const iconEl = document.getElementById('thing-title-icon');
Expand Down