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

API break between 0.1.1 and 0.1.2 #52

Open
ecksun opened this issue Feb 16, 2015 · 5 comments
Open

API break between 0.1.1 and 0.1.2 #52

ecksun opened this issue Feb 16, 2015 · 5 comments

Comments

@ecksun
Copy link
Contributor

ecksun commented Feb 16, 2015

I recently upgraded from dbus-native 0.1.1 to 0.1.2 and now get crashes that
appear to be because of a change in API.

Bisecting the issue I found that my code stopped working with commit
f21eab6

I deviced the following small test to illustrate the difference:

'use strict';
var assert = require('assert');
var dbus = require('dbus-native');

var sysbus = dbus.systemBus();
var service = sysbus.getService('org.freedesktop.NetworkManager');

service.getInterface('/org/freedesktop/NetworkManager', 'org.freedesktop.DBus.Properties', function(err, propIface) {
    assert.ifError(err);
    propIface.GetAll('org.freedesktop.NetworkManager', function(err, props) {
        assert.ifError(err);
        console.log(require('util').inspect(props, { depth: null }));
    });
});

With version 0.1.1 I get the following output:

[ [ 'Devices',
    [ [ '/org/freedesktop/NetworkManager/Devices/0',
        '/org/freedesktop/NetworkManager/Devices/1',
        '/org/freedesktop/NetworkManager/Devices/2',
        '/org/freedesktop/NetworkManager/Devices/4',
        '/org/freedesktop/NetworkManager/Devices/6',
        '/org/freedesktop/NetworkManager/Devices/7' ] ] ],
  [ 'NetworkingEnabled', [ true ] ],
  [ 'WirelessEnabled', [ true ] ],
  [ 'WirelessHardwareEnabled', [ true ] ],
  [ 'WwanEnabled', [ true ] ],
  [ 'WwanHardwareEnabled', [ true ] ],
  [ 'WimaxEnabled', [ true ] ],
  [ 'WimaxHardwareEnabled', [ true ] ],
  [ 'ActiveConnections',
    [ [ '/org/freedesktop/NetworkManager/ActiveConnection/23',
        '/org/freedesktop/NetworkManager/ActiveConnection/3',
        '/org/freedesktop/NetworkManager/ActiveConnection/2',
        '/org/freedesktop/NetworkManager/ActiveConnection/1',
        '/org/freedesktop/NetworkManager/ActiveConnection/0' ] ] ],
  [ 'PrimaryConnection',
    [ '/org/freedesktop/NetworkManager/ActiveConnection/1' ] ],
  [ 'ActivatingConnection', [ '/' ] ],
  [ 'Startup', [ false ] ],
  [ 'Version', [ '0.9.10.0' ] ],
  [ 'State', [ 70 ] ],
  [ 'Connectivity', [ 4 ] ] ]

While with version 0.1.2 I get the following:

[ [ 'Devices',
    [ [ { type: 'a', child: [ { type: 'o', child: [] } ] } ],
      [ [ '/org/freedesktop/NetworkManager/Devices/0',
          '/org/freedesktop/NetworkManager/Devices/1',
          '/org/freedesktop/NetworkManager/Devices/2',
          '/org/freedesktop/NetworkManager/Devices/4',
          '/org/freedesktop/NetworkManager/Devices/6',
          '/org/freedesktop/NetworkManager/Devices/7' ] ] ] ],
  [ 'NetworkingEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WirelessEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WirelessHardwareEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WwanEnabled', [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WwanHardwareEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WimaxEnabled', [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'WimaxHardwareEnabled',
    [ [ { type: 'b', child: [] } ], [ true ] ] ],
  [ 'ActiveConnections',
    [ [ { type: 'a', child: [ { type: 'o', child: [] } ] } ],
      [ [ '/org/freedesktop/NetworkManager/ActiveConnection/23',
          '/org/freedesktop/NetworkManager/ActiveConnection/3',
          '/org/freedesktop/NetworkManager/ActiveConnection/2',
          '/org/freedesktop/NetworkManager/ActiveConnection/1',
          '/org/freedesktop/NetworkManager/ActiveConnection/0' ] ] ] ],
  [ 'PrimaryConnection',
    [ [ { type: 'o', child: [] } ],
      [ '/org/freedesktop/NetworkManager/ActiveConnection/1' ] ] ],
  [ 'ActivatingConnection',
    [ [ { type: 'o', child: [] } ], [ '/' ] ] ],
  [ 'Startup', [ [ { type: 'b', child: [] } ], [ false ] ] ],
  [ 'Version', [ [ { type: 's', child: [] } ], [ '0.9.10.0' ] ] ],
  [ 'State', [ [ { type: 'u', child: [] } ], [ 70 ] ] ],
  [ 'Connectivity', [ [ { type: 'u', child: [] } ], [ 4 ] ] ] ]

Obiously this is a breaking change, my question is if this is an intentional
API break or if I should consider it a bug?

If it is an intentional break I think we should add some documentation
detailing the new API.

@sidorares
Copy link
Owner

I think current ( 0.1.2 ) behaviour is consistent with pre 0.1 and what you had with 0.1.1 was an accidental bug ( most of the deserialising code is rewritten by #41 )

The idea is that by default given message top level signature and message data you should be able to generate back exactly the same binary representation of the message. This is something not necessary to most users, so that default might be changed ( but still exist behind a flag ) in favour of more compact. This is especially important when you handle json-like list of properties where "full" view of a(sv) is really difficult to read or write.

Documentation is probably on top of my priority list and any help would be appreciated

In short:

  • current behaviour is intended ( but might change ) If it changes that would be major version bump
  • really need to document dbus type system mapping to js
  • introduce 'lightweight' mapping which is not 1 to 1 binary but much shorter ( strings mapped to strings, numeric types mapped based on range, a(sv) to objects

For example, if we switch to 'lightweight' results by default api might be like this

// 0.1.1 - like behaviour:
var sysbus = dbus.systemBus({ preserveTypes: true });
// new default: compact (de)searialisation of variands
var sysbus1 = dbus.systemBus();

wdyt?

@ecksun
Copy link
Contributor Author

ecksun commented Feb 17, 2015

Aha, I see.

Im not very familiar with the details of dbus, could you give an example of a case where data is lost when converting from dbus types to javascript types?

In your example, you write that the default should be the default, with default do you mean the behaviour in 0.1.1? Also, would preserveTypes mean to not loose any data, that is to preserve the dbus types or does it mean that we preserver javascript types?

However I think the idea in general is good, I guess you could also expose some utility to convert from one format to the other, dunno if thats better though.

var niceSysBus = new dbus.NiceBus(dbus.systemBus());

In order to give a better response I think I need to study the dbus type system some more, do you have any good references I could read?

@sidorares
Copy link
Owner

If you have "ay" array and JS data like [ 1, 2, 3, "aaa" ] you can only guess actual type of elements - first 3 can be any of y,n,q,i,u,x,d,t types and fourth any of s,o,g. If data is

[  
  [ [ { type: 'b', child: [] } ], [ true ] ], 
  [ [ { type: 'i', child: [] } ], [ 123 ] ], 
  [ [ { type: 'y', child: [] } ], [ 123 ] ],
  [ [ { type: 's', child: [] } ], [ "aaa"] ],
]

then you know exactly how to serialise your data

I'll try to describe dbus type system in details (hopefully copy something from here to readme)

D-bus uses simple string type representation to describe how message should be serialised to and from raw binary data. Each character in the "signature" represents simple type, start of array/struct/hash or variant. Simple types:

name encoding js example
y unsigned byte 123
b unsigned 32 bit true
n signed 16 bit -300
q unsigned 16 bit 65000
i signed 32 bit -123000
u unsigned 32 bit 123000
x signed 64 bit 123
t unsigned 64 bit 123
d IEEE 754 double 3.14159
h unsigned 32 bit unix fd, not supported

String-like types are prefixed with length ( 8 bit for "g" and 32 for "o" and "s" )

name encoding js example
s,o 32 bit unsigned int + utf8 bytes + '\0' "test string"
g 8 bit unsigned int + ascii bytes + '\0' "iisa(ii)"

arrays, structs and maps:

array signature is "a" followed by full signature ( simple type, structure, array, struct or variant )

signature JS example
ai [1, 2, 3]
aai [ [1,2,3], [], [5,6]]

structure is described with "(" and ")" and a list if full types in between. Often used to describe single type for array or tuple element

signature JS example
i(is) [ 1, [2, "baz" ] ]
a(ii) [ [1, 2], [3, 4] ]
a(is) [ [1, "foo"], [2, "bar" ]

structure described with "{" and "}" is similar to "()" with exception that it can only contain have exactly two values - it's a tuple. Usually goes together with "a" to represent map

signature JS example
a{si} [ [ "foo", 1 ], [ "bar", 2 ] ]

Variants are prefixed with signature followed by encoded value

signature JS example
ay [ [ { type: 's', child: [] } ], [ '0.9.10.0' ] , [ { type: 'b', child: [] } ], [ false ] ] ],

( to be continued ... )

@ecksun
Copy link
Contributor Author

ecksun commented Feb 18, 2015

Aha, I see, I did not think the dbus type system was so detailed. That explains quite a lot, thank you :)

I also think that you can steal most of this and put it in the documenation.

When I considered this problem I originaly did not think of the case converting from js types to dbus types (which isn't fully possible). However converting from dbus types from js types should work fine.

With that in mind though, I guess its not ever really possible to convert from js types to dbus types in a good manner, that is the "preserveTypes" mode doesnt work very well going from js -> dbus, right?

However when reading (which is what I mostly do) it should be fine providing two difrerent modes (or a way to convert from dbus types to js types).

Basically, what I will do when I get time is to rewrite my dbus-native wrapper to convert from dbus-types to javascript types. It would be nice if that utility could be provided by dbus-native.

@sidorares
Copy link
Owner

Automatic conversion draft:

JS -> dbus serialisation

signature JS short JS full
av [ 1, 2, true, "test" ] [ [ { type: 'd', child: [] } ], [ 1 ] ], [ { type: 'd', child: [] } ], [ 2 ] ], [ { type: 'b', child: [] } ], [ 1 ] ], [ { type: 's', child: [] } ], [ "test ] ] ]
a{sv} { test: "foo" } [ [ "test", [ { type: 's', child: [] } ], [ "test ] ] ] ]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants