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

WIP: reworking data model section #93

Closed
wants to merge 5 commits into from
Closed

Conversation

mlagally
Copy link
Contributor

@mlagally mlagally commented Jul 29, 2021

@mlagally mlagally changed the title WIP: reworking data model section reworking data model section Jul 29, 2021
Copy link
Contributor

@mmccool mmccool left a comment

Choose a reason for hiding this comment

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

General thoughts:

  • Moving description to "RECOMMENDED" is good and reasonable
  • A justification should be added for why "title" is mandatory, which will also help people understand what it should contain. I would expect this to be used to generate UIs automatically (e.g. dashboards), so it should be appropriate for that, and there should be a comment to that effect. This also justifies the length limitation.
  • I am confused about the object nesting limitations. At the start of the section it now says it is limited to 1 (and even has an example of RGB objects broken down into elements rather than a nested object) but later there is a section saying the depth is 5, which we agreed was a reasonable compromise covering most use cases. The justification for the nesting depth of 1 is relational databases, but really, I would expect there are approaches now for generating reversible name mappings (e.g. putting names like color.rgb.blue as the keys in the database) that would address the problem of flattening and unflattening...

I don't think we can merge this until the the object nesting limitation confusion is resolved. We previously agreed to a nesting depth of 5 and think that's what this section should say.

@mmccool
Copy link
Contributor

mmccool commented Jul 29, 2021

One further thought: languages. Somewhere it says that the preferred language is English.

  • This is reasonable for API documentation; internal "technical" documentation is typically in English
  • It's not reasonable for information meant to be used on a UI, i.e. (perhaps) "title". I think it would be reasonable for even small devices to support language negotiation when fetching a TD (something I need to think about as part of the Discovery spec) and then return values for title in that language, maybe omit descriptions, etc.
  • In practice, if "description" is mostly used by API developers and "title" is intended for UIs, then it's possible that TDs might be developed with multiple languages for "title" but only English for "description". In which case, if I do language negotiation and ask for Czech and there is a title in that language but not a description, it would be ok to omit the description in the result. The fact that description is optional in the profile in the current PR at least makes this pattern possible.

Anyway, in summary, I think we need to think a bit more about internationalization requirements.

Copy link
Member

@benfrancis benfrancis left a comment

Choose a reason for hiding this comment

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

I welcome the reduced set of constraints, but I still see nothing here which either contributes to interoperability or couldn't be moved to the Thing Description specification.

My recommendation is still to remove the entire section and redistribute parts of its content to other sections of the WoT Profile specification or to the Thing Description specification, as proposed in #78.

5.1 WoT Core Data Model

The core data model incorporates the data model defined by chapter 5 of the Thing Description specification.

I suggest that the data model in the Thing Description specification should be the core data model.

5.1.1 General

The following rules are applicable to multiple classes of the WoT Thing Description Specification,
as they provide clearer semantics, improved readability and simplified processing on resource constrained devices.

As I understand it we have now agreed that the goal of the core profile is interoperability, not improved readability and processing on resource constrained devices which cause conflicting requirements.

5.1.1.1 Mandatory fields

the fields title and description are MANDATORY for Things, Property Affordances, Action Affordances, Event Affordances and Data Schemas.

This does not contribute to interoperability.

It is possible to have empty values for these fields, if, for specific purposes it is not desired to provide documentation, however this is NOT RECOMMENDED and the conscious decision is obvious from the TD.

I see no benefit of forcing developers to insert empty fields in their Thing Descriptions, what does this achieve?

5.1.1.2 Length and Value Limits

The length of id , description and descriptions values is limited to 512 characters.
The length of title and titles values is limited to 64 characters.

How does setting arbitrary limits on the length of fields help interoperability? If a device has enough resources to run an HTTP server serving JSON resources then I don't think a few characters of text are going to make much difference. Note that the size of a device's metadata is likely to be at least one order of magnitude smaller than the size of the data it generates.

Does this mean that if a Consumer encounters a Web Thing with a title 65 characters long that it should refuse to communicate with it?

Where a type permits using an array of string or a string , an array of string MUST be used.

This kind of constraint will only result in unnecessarily larger Thing Descriptions, which seems to be working against the previous constraints. It's really not that much work for a Consumer to check the type of a field before processing it.

All elements of an enum MUST be either string or number . Different types in a single enum are NOT PERMITTED.

What is the reason for forbidding an enum of objects or arrays?

5.1.2

5.1.2.1 Mandatory fields

To provide minimum interoperability, the following metadata fields of a Thing MUST be contained in a compliant Thing Description

How does making title, id and version mandatory aid interoperability?

5.1.2.2 Recommended practice

The following metadata fields of a Thing SHOULD be contained in a compliant Thing Description:

What makes these fields recommended for a Web Thing conforming to the Core Profile, but not a device which doesn't conform to the Core Profile? If these recommendations apply to all devices then I suggest moving them to the Thing Description specification.

It is RECOMMENDED to use the value "" for strings, where the value cannot be determined.

It would be a lot simpler to just allow these fields to be optional, why waste resources providing empty fields?

If a Thing Description is used solely within a company, the email address of the developer SHOULD be used in the support field, if the Thing Description is provided externally, a support email address SHOULD be used.

This is strange recommendation, I suggest removing it.

5.1.3 Data Schema

This section defines a subset of the class DataSchema that can be processed on resource-constrained devices.

We have agreed that this is no longer a goal of the Core Profile.

5.1.3.1 Data Schema Constraints

The Core Data Model restricts the use of arrays and objects to the top level of Data Schemas, i.e. only a one-level hierarchy is permitted. The members of a top level object or array MUST NOT be array or object types.

I suggest this is too restrictive. Complex devices with nested data structures should be allowed to use the Core Profile.

Many enterprise services and applications are based on (relational) databases, where individual property values are stored. Of course databases can also store objects (e.g. encoded as a JSON string), however this will prevent processing by other enterprise applications.

This is not a good reason in my opinion. Enterprise services could choose to use different database technologies where that is appropriate, and where a relational database is necessary there are approaches for storing nested data structures in relational databases.

The following fields MUST be contained in a DataSchema:
title

Why?

The type value MUST NOT be null

We have come across use cases in WebThings where this is necessary, e.g. where the value of a property is an image or video.

5.1.3.2 Recommended Practice

The following fields SHOULD be contained in a DataSchema:
description

Move to TD specification?

Usage of the english language is recommended

Why? It is possible to provide multiple descriptions in different languages using the descriptions member, or by using HTTP content negotation to request a particular language. Why should English be the primary language?

5.1.4 Property Affordance

5.1.4.1 Mandatory fields

The following property fields MUST be contained in the properties element of a Profile compliant TD:
title

Why?

5.1.4.2 Additional Constraints

Parsing of a deeply nested structure is not possible on resource constrained devices.

We have agreed that supporting processing on constrained devices is not a goal of the Core Profile.

each property MUST NOT exceed a maximum depth of 5 levels of nested array or object elements. It is RECOMMENDED to keep the nesting of these elements below 4.

This contradicts 5.1.3.1 and I don't see the value in setting an arbitrary limit. If a Consumer can traverse 5 levels then it can traverse 6 levels.

The following additional constraints MUST be applied to the Property Affordances of a Thing Description conforming to the Core Profile:
const | anyType | MUST NOT be used

Why?

enum | array of simple type | Values of enums MAY only be simple types. Handling of any type is too complex to implement on resource constrained devices

Non-goal.

forms | array of Forms | The Array of Form of each property MUST contain only a single endpoint for each operation readproperty, writeproperty , observeproperty, unobserveproperty.

This works against interoperability because it prevents a Web Thing from conforming with multiple profiles or from conforming with the Core Profile whilst also supporting other protocols. For example a Web Thing could not conform to both the Core Profile and a Constrained Profile at the same time, and could not provide a WebSocket endpoint whilst also providing an HTTP endpoint for the same interaction affordance in order to be compliant with the Core Profile.

oneOf | string | The DataSchema field oneOf does not make sense for properties and MUST NOT be used.

Why not? If this is the case then this should be defined in the Thing Description specification.

5.1.4.3 Recommended Practice

It is highly RECOMMENDED to always specify a unit, if a value has a metric

What are the recommended set of acceptable values? If this section constrained conformant Web Things to a particular ontology or set of ontologies for units then it could actually be useful for interoperability.

5.1.5 Action Affordances

5.1.5.1 Mandatory fields

The following fields MUST be contained in an action element of an Core Profile compliant TD:
title | string | unique name among all actions

Why?

input | array of DataSchema | all elements of the subclasses objectSchema and dataSchema MUST only contain simple types.

Some actions don't require an input because all the information needed is contained in the action name, e.g. "reboot".

Some actions need nested data structures in their input.

output | array of DataSchema | all elements of the subclasses objectSchema and dataSchema MUST only contain simple types.

Some actions don't need to provide an output, e.g. "fade".

Some actions need nested data structures in their output.

5.1.5.2 Additional Constraints

The elements of the DataSchema subclasses ArraySchema and ObjectSchema for the fields input and output are restricted to simple types in a Thing Description conforming to the Core Data Model. Without this limitation a higher implementation burden would be put on resource constrained devices

Non-goal.

The following additional constraints MUST be applied to the Interaction Affordances of a Thing Description conforming to the Core Data Model:
forms | array of Forms | The Array of Form of each action MUST contain only a single endpoint.

Harmful to interoperability.

oneOf | string | The DataSchema field oneOf does not make sense for properties and MUST NOT be used.

Typo properties -> actions. Why? Should be moved to the Thing Description specification if true.

uriVariables | Map of DataSchema | uriVariables MUST NOT be used.

This may prevent the Core Profile Protocol Binding from being compliant with the Core Profile Data Model if we land #89

5.1.6 Event Affordance

A Thing may provide more than one event mechanism to enable a variety of consumers.

Why is this true of events, but not properties or actions? E.g. using WebSockets to observe a property or the status of an ongoing action.

5.1.6.1 Mandatory fields

The following fields MUST be present in an event element of a Core TD:
title | string | unique name among all events

Why?

description | string | human readable description

Why?

data | set of DataSchema instances in a JSON object | only the DataSchema subclasses booleanSchema, IntegerSchema, NumberSchema, StringSchema are permitted

Some events do not need to provide a payload as the name provides all of the information, e.g. "rebooting".

Some events need nested data structures in their payloads.

5.1.6.2 Additional Constraints

The following additional constraints MUST be applied to the Event Affordances of a WoT Thing Description conforming to the profile:
forms | array of Forms | The Array of Form of each event MUST contain only a single endpoint.

Harmful to interoperability.

uriVariables | Map of DataSchema | uriVariables MUST NOT be used.

Some Web Things need to use URI variables for events, e.g. https://w3c.github.io/wot-discovery/#directory-thing-description

5.1.7 Forms

A Thing may provide more than one event mechanism to enable a variety of consumers.

I don't think this sentence is meant to be here?

5.1.7.1 Mandatory fields

This section has been erroneously copied from the Events section.

5.1.7.2 Additional constraints

security | string or Array of string | security at form level MUST NOT be used.

Security at the Form level may be necessary for some Web Things, e.g. if it provides a WebSocket endpoint where an API token needs to be provided in a URL rather than an HTTP Authorisation header.

5.1.8 Links

The Core Data Model does not put additional constraints or requirements on links.

I suggest that the Core Profile could provide a set of link relation types which Web Things MAY use and Consumers MUST support.

5.1.9 Security

I suggest moving this content into a separate Security section of the Core Profile section, as in #78

@@ -1510,7 +1581,7 @@ <h4>Recommended Practice</h4>

<!-- Protocol Binding -->
<section id="protocol-binding">
<h3>Protocol Binding</h3>
<h3>HTTP 1.1 Protocol Binding</h3>
Copy link
Member

Choose a reason for hiding this comment

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

Are you expecting there to be a separate HTTP/2 protocol binding? My assumption is that they would be the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point the version number is just added to avoid any ambiguities.
I'm not sure if HTTP/2 will require any other changes beyond updated references.

@egekorkan
Copy link
Contributor

I agree with all the points that @benfrancis made above. The main problem that is still present since #37 is the not well-argumented use of constraints everywhere.

I have also seen that the changes are not a lot when I look into PR Review Diff highlighted in yellow. So this PR is OK but merging this or not will not address my concerns.

Specific Comments

Single forms per operation

One explanation to @benfrancis comment about the reason for affordances having a single form is to avoid cases like:

forms:[
{
  "href":"http://192.168.1.24:8080/actions/fade",
  "op":"invokeaction"
},
{
  "href":"https://myWoT.com:80/actions/fade",
  "op":"invokeaction"
}
]

That is, sadly, just what I remember from some discussions somewhere. IF that is indeed the reason, it should be documented.

Version

I somehow didn't see this in the FPWD. Making the version mandatory does not help much. Both examples below are following the core profile:

{
    ...
    "version": { "instance": "1.2.1" },
    ...
}
{
    ...
    "version": { "instance": "tagliatelle" },
    ...
}

@mlagally mlagally changed the title reworking data model section WIP: reworking data model section Dec 6, 2021
@mlagally
Copy link
Contributor Author

mlagally commented Dec 6, 2021

The latest draft is still work in progress - All references to resource constrained devices have been removed and corresponding constraints were deleted.

The current draft is WIP and needs more refinement, these sections are marked with editor's notes.
Currently there's a merge confict, so I recommend to review the preview or the diff.

@mlagally
Copy link
Contributor Author

mlagally commented Dec 6, 2021

  • A justification should be added for why "title" is mandatory, which will also help people understand what it should contain. I would expect this to be used to generate UIs automatically (e.g. dashboards), so it should be appropriate for that, and there should be a comment to that effect. This also justifies the length limitation.

I added rationales to all constraints.

  • I am confused about the object nesting limitations. At the start of the section it now says it is limited to 1 (and even has an example of RGB objects broken down into elements rather than a nested object) but later there is a section saying the depth is 5, which we agreed was a reasonable compromise covering most use cases. The justification for the nesting depth of 1 is relational databases, but really, I would expect there are approaches now for generating reversible name mappings (e.g. putting names like color.rgb.blue as the keys in the database) that would address the problem of flattening and unflattening...

I completely removed the depth constraints which are no longer required.

@mlagally
Copy link
Contributor Author

mlagally commented Dec 6, 2021

@egekorkan

Single forms per operation

One explanation to @benfrancis comment about the reason for affordances having a single form is to avoid cases like:

forms:[
{
  "href":"http://192.168.1.24:8080/actions/fade",
  "op":"invokeaction"
},
{
  "href":"https://myWoT.com:80/actions/fade",
  "op":"invokeaction"
}
]

That is, sadly, just what I remember from some discussions somewhere. IF that is indeed the reason, it should be documented.

We have two alternatives:

  1. specify how multiple forms for the same affordance, operation, protocol are treated. I cannot think of an algorithm that can do that in a reasonable way.
  2. constrain to a single form per affordance+operation+protocol

@danielpeintner's comment about a profile for different protocols:
This would be possible - for each combination of affordance+operations there must only be a single form PER PROTOCOL

@benfrancis I still have to review your comments in detail, but I believe I have addressed many of them.
I have also added language that excludes hybrid operations e.g. combined read+write of a property, combined subscribe and unsubscribe and several other constraints I consider reasonable.

Copy link
Member

@benfrancis benfrancis left a comment

Choose a reason for hiding this comment

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

Hi @mlagally, I welcome the effort to further reduce the set of constraints, but I'm afraid this latest version still does not come anywhere near addressing my concerns.

I could provide lots of detailed feedback on the new text, but there probably isn't much point if you haven't yet had chance to review my previous feedback from July and we are also still trying to reach agreement on the goals of the specification (#73).

Fundamentally, this version still includes constraints which mix up interoperability, human readability (e.g. mandatory titles) and resource constrained devices (e.g. arbitrary character limits) and I disagree with the vast majority of the text.

The only parts I think could potentially be useful for an interoperability profile are the Security and Links sections which I have proposed (#125) moving out into their own sections, and parts which haven't been written yet like date formats and unit constraints.

Please see #125 (comment) for my proposed course of action, which can be summarised as:

  1. Formally agree on the goals and scope of the specification in Refine Goals and Scope #73
  2. Start from a blank slate and decide whether any data model constraints beyond what's already defined in the Thing Description specification would contribute to those goals

@mlagally
Copy link
Contributor Author

Arch call on July 20th:
Agree closing - comments are preserved.

@mlagally mlagally closed this Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants