telefonicaid / fiware-orion

Context Broker and CEF building block for context data management, providing NGSI interfaces.
https://github.com/telefonicaid/fiware-orion/blob/master/doc/manuals/orion-api.md
GNU Affero General Public License v3.0
210 stars 265 forks source link

API v2 wish list #768

Closed fgalan closed 6 years ago

fgalan commented 9 years ago

After several years of experience with the NGSI "clasical" API (the one we refer with "v1" in the URLs) and several concessions needed to mantain backward compatibility along the time, PR after PR, let's thought about if we could re-design the API without backward compatibilty legacy and call it "v2" :)

This issue is a placeholder to keep all the ideas around. You are encourage to participate in the discussion adding comment to this issue, specially if you are an intense user of NGSI. Periodically, I will review the issue, consolidating comment into items in the issue body (and removing the comments).

List:

kzangeli commented 9 years ago

Attributes should have two more fields (at least):

With this addition, the forwarding of updates and queries we have implemented for NGSI10 would no longer depend on the registrations from NGSI9. That is not the only advantage ...

kzangeli commented 9 years ago

Just a detail: registration responses in NGSI9 have the field 'registrationId' as mandatory. What if the registration couldn't be performed (e.g. due to a database error). Right now we return an registrationId of '000000000000000000000000000000000' if that happens. Doesn't make much sense ...

kzangeli commented 9 years ago

Wouldn't it be nice (apart from all other security we can add from outside of the broker) if every attribute had permissions similar to file-permissions in a unix file system?

{ read, write, execute } for { owner, group, world }

Of course, we'd have to manage users and groups to implement this. No problem! :-)

kzangeli commented 9 years ago

Some convenience operations (especially of ngsi10 query type) have the ability to return only one unique entity (context element), while the query could have more than one entity as a result. What we do in these cases is that we select the first hit from the query and we return it as response.

These convops should be modified to return a list of entities.

kzangeli commented 9 years ago

Convenience Operations are operations that do the same operation as a Standard Operation, but with simpler payload (or no payload at all).

Thus, each conv-op has a corresponding std-op.

All conv-ops should respond with the same response-type as their corresponding std-op.

kzangeli commented 9 years ago

In v1, all standard operations use the verb POST. In some cases, UPDATE. APPEND, or DELETE is later added in the payload. This is not very good and we should use the proper verbs for each operation for v2.

E.g. for updates, the updateType should not be in the payload, but reflected in the request verb instead.

kzangeli commented 9 years ago

In v1, context provider forwarding depend on NGSI9 registrations to identify the Context Providers of the attributes.

In v2 I would propose to remove this strange way of identifying the Context Providers and just put that information where it should have been from the beginning - as a part of the ContextAttribute.

This enhancement makes us capable to easily implement UpdateContext/APPEND and UpdateContext/DELETE forwarded to Context Providers as well. Right now (in v1) we only support forwarding for UpdateContext/UPDATE.

kzangeli commented 9 years ago

In v1 we cannot support the URI params "entity::type EMPTY" and "entity::type NON-EMPTY" as UpdateContextRequest doesn't contain Restriction.

Unless we forbid EntityId::type to be empty (my vote), we should add a way to be able to pass these flags in the v2-equivalent structure 'UpdateContextRequest'. This will probably not be the only restriction we'd want in updates, so Restriction should be added to UpdateContextRequest, just in case.

fgalan commented 9 years ago

Having entities without a type and querying without a type with special semantics is a mess. Thus, it would be better simply to have pattern for types (how to encode this in NGSI is pending, e.g. "isPatternType"). Moreover, even allowing entities with type "" is a bad idea, maybe entity type should be mandatory.

fgalan commented 9 years ago

The attribute ID stuff is confusing and easy to implement with regular attributes (e.g. just use ":" to add the ID after the base attribute name). Remove it from v2.

kzangeli commented 9 years ago

About entities without type: I agree 100% Let's make entity::type mandatory in v2, and the same for attributes and even metadata, I think. Why make life more complicated on purpose?

fgalan commented 9 years ago

Remove the operations with "/attributes" which are exactly equal to the same operations without "/attributes". It doesn't make sense to have such duplication in URLs and operations.

LeandroGuillen commented 9 years ago

Have entities inform their service path, maybe as a new field, metadata, ...

This opens up new high tech possibilities like changing service paths :)

[Fermin] I think that entities are "informing" about their service path in the Fiware-ServicePath header included in queryContextResponse...

dmoranj commented 9 years ago

Changing service path may be a problematic feature, as other GEs might be using it in data structures that cannot be easily changed. We better think about this with care. El 12/03/2015 18:58, "Leandro Guillén" notifications@github.com escribió:

Have entities inform their service path, maybe as a new field, metadata, ...

This opens up new high tech possibilities like changing service paths :)

— Reply to this email directly or view it on GitHub https://github.com/telefonicaid/fiware-orion/issues/768#issuecomment-78548494 .

fgalan commented 9 years ago

The operation to get the value of a single attribute is highly verbose:

GET <cb_host>:<cb_port>/v1/contextEntities/type/Restaurant/id/LeBistro/attributes/average_scoring

{
  "attributes" : [
  {
    "name": "average_scoring",
    "type": "float",
    "value": "3.2"
  }
  ],
  "statusCode" : {
    "code" : "200",
    "reasonPhrase" : "OK"
  }
} 

It could be simplified to just:

GET <cb_host>:<cb_port>/v1/contextEntities/type/Restaurant/id/LeBistro/attributes/average_scoring

{
   "type": "float",
   "value" : "4.2"
}

and, in that sense, be homogenous with the operation to update the attribute:

PUT <cb_host>:<cb_port>/v1/contextEntities/type/Restaurant/id/LeBistro/attributes/average_scoring
{
   "value" : "4.2"
}
Adirio commented 9 years ago

The wish proposed by @fgalan on 7th of April conflict with the first one made by @kzangeli the 23rd of February. One of the two aproaches should be choosen, verbose but common for both standard and convenience operations, or simplified for convenience operations but diferent from the standard ones.

If updateActions don't get removed, they should be properly defined. A forth updateAction should be added (proposed name CREATE) that works as the actual APPEND but not updating if it already exists.

  1. CREATE:
    • Entity does not exist: create entity and attributes with their initial value.
    • Attribute does not exist: create attributes with their initial value.
    • Attribute already exists: do nothing and send error (attribute already exists).
  2. UPDATE:
    • Entity does not exist: do nothing and send error (entity does not exist).
    • Attribute does not exist: do nothing and send error (attribute does not exist).
    • Attribute already exists: update attribute's value.
  3. APPEND:
    • Entity does not exist: create entity and attributes with their initial value.
    • Attribute does not exist: create attributes with their initial value.
    • Attribute already exists: update attribute's value.
  4. DELETE
    • Entity does not exist: do nothing and send error (entity does not exist).
    • Attribute does not exist: do nothing and send error (attribute does not exist).
    • Attribute already exists: delete attribute.

If they get swapped with the request verb, POST should retain CREATE behaviour described above, PUT should have the same behaviour as UPDATE and DELETE would keep DELETE's behaviour. The APPEND behaviour should not be used by default nor by POST nor PUT requests, it should be only be used if a specific header is set for one or both of the previous request verbs.

Explanation on why CREATE should replace APPEND as the POST action: Most of the times when you create a new entity you set some initial values, which may even be empty, that you will modify. If the attribute already exists and someone is already modifying it's value, updating a valid value for an initial value would turn the value non-valid until the next update action arrives. This behaviour should still be offered as non-dafult as it may be useful for some applications but in general the initial value set on a POST/CREATE action should not modify a real value set by a previous PUT/UPDATE action. The same way you don't set a value to empty to delete it (you use the DELETE action), CREATE and UPDATE should be 2 separate actions. In v1 UPDATE is not used as you get the same behaviour with APPEND, so separating this actions would also allow better error handling.

kzangeli commented 9 years ago

Thank you very much for your comment, @Adirio. I pretty much agree to what you say in your post. Two comments:

  1. The new proposed behavior on "APPEND" is probably not according to the NGSI standard, which is not all that important for what we call v2. I would call it "CREATE" (or "INSERT") instead of "APPEND" though. It is pretty much what it is.
  2. The third type 'APPEND&INSERT', isn't this like what's called UPSERT in some database?
Adirio commented 9 years ago

@kzangeli names in my post are just place holders, substitute them for whatever you like.

I actually like the CREATE name (it also has 6 letters as the other 3) and the one I called APPEND&UPDATE could stay as APPEND. I modified my previous comment to reflect this new names.

Adirio commented 9 years ago

To add to the wish list, an following my previous posts, the behaviour of an existing ONCHANGE subscription when CREATE or UPDATE are executed should be defined.

In my opinion, CREATE should be used as an instantiating action, so it shouldn't trigger an ONCHANGE notification. Actual behaviour is like this if the attribute already exists as not modifying the value as specced 3 comments above wouldn't trigger an ONCHANGE notification but if the subscription exists but the attribute doesn't, what can be done, creating a new attribute would trigger an ONCHANGE notification and this behaviour should be changed. The CREATE action is ment to be used when you know you need some entity's attribute but you don't know yet its first value, so you set an initial value that shouldn't be sent to subscribers that are looking for real values. In the other hand, APPEND actions should send an ONCHANGE notification the same way as UPDATE do, as APPEND is ment to be used as an UPDATE for any data that doesn't care if it already existed while UPDATE is ment to be used when you need to have created the attribute prior to the UPDATE.

This way the 3 verbs should have a sense: CREATE a sensor as I'll be using it but not yet have its output. UPDATE a sensor when i get an output value from it. DELETE a sensor when you don't want more outputs from that sensor. APPEND a sensor when I don't care if it exists or not, I have an output value and I'll submit it; or when I want to create a sensor but I already have an output value from it.

A sensor is used as an example because I wasn't sure if my words were clear.

fgalan commented 6 years ago

This issue has been considered not essential to be included in final version of NGSIv2, so it will be closed.

CC: @jmcanterafonseca