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

JDK1.8 json format #3051

Closed
henryzhao81 opened this issue Nov 11, 2014 · 23 comments
Closed

JDK1.8 json format #3051

henryzhao81 opened this issue Nov 11, 2014 · 23 comments
Assignees
Labels

Comments

@henryzhao81
Copy link
Contributor

Follow up issue : #2983

create function toJSON,

toJSON :

try {
return JSON.parse("{"@rid":"#51:2051"}");
} catch(e) {
}

Runing under JDK1.7
[
{
"@Rid": "#51:2051"
}
]

Runing under JDK1.8
[
{
"@type": "d",
"@Version": 0,
"value": {
"@Rid": "#51:2051"
}
}
]

@mattaylor
Copy link
Contributor

this affects every response from Javascript functions. - they are always wrapped in a 'value' object. It would be better to return the same value as with 1.7 - or else wrap the response in an ODocument using fromJSON.

@mattaylor
Copy link
Contributor

Any more thoughts on this. Currently the response from javascript functions is deeply inconsistent and unwelidly. Can we try to stick to something like..
a) if the function returns an array then return the array as the results property in the response object
b) if the function returns an object then return the object in a single item array as the results property of the response object
c) if the function returns a scalar then return the result as the 'value' of an object in a single item array as the results property of the response object

@luigidellaquila
Copy link
Member

this issue comes from this #2388
It seems that it was the expected behavior at that time...
IMHO it's wrong and it should be fixed, I will investigate more and let you know

luigidellaquila added a commit that referenced this issue Nov 26, 2014
@luigidellaquila
Copy link
Member

I fixed this, now the behavior is consistent between java7 and java8.
I also made the overall behavior more consistent, now simple js objects are returned as normal objects, eg. no envelope document with "value" field is created anymore.
The only case when it happens is when a function returns a simple value (number or string)

@mattaylor
Copy link
Contributor

How about arrays? Are these returned as per query resu
lts or are are these wrapped in a value param

@phpnode
Copy link
Contributor

phpnode commented Nov 26, 2014

@luigidellaquila why differentiate? either always envelope or never envelope but alternating between the two makes things really painful for consumers.

@luigidellaquila
Copy link
Member

Hi Charles,

let's talk about this, I don't like current implementation either. My
proposal is this:

function:
return 1

result:

{"result": 1}


function:
return "foo"

result:

{"result": "foo"}


function:
return {"a":1, "b":3}

result:

{"result": {"a":1, "b":3}}


function:
return [1, 2, 3]

result:

{"result": [1, 2, 3]}


function:
return db.query("select from OUser")

result:

[
{
"@type": "d",
"@Rid": "#5:0",
"@Version": 2,
"@Class": "OUser",
"name": "admin",
"password": "...",
"status": "ACTIVE",
"roles": [
"#4:0"
],
"@fieldTypes": "roles=n"
},
{
"@type": "d",
"@Rid": "#5:1",
"@Version": 1,
"@Class": "OUser",
"name": "reader",
"password":
"{SHA-256}3D0941964AA3EBDCB00CCEF58B1BB399F9F898465E9886D5AEC7F31090A0FB30",
"status": "ACTIVE",
"roles": [
"#4:1"
],
"@fieldTypes": "roles=n"
}
]

IMHO it's the most consistent way to manage function results.

The real problem is that this is completely different from current
implementation, so we could break many client applications whose code is
based on previous conventions (even though they were deeply inconsistent,
even for the same function between different java versions).

I'd like to have a feedback from as many users as possible to take a final
decision, because it could be a source of problems for many of you.

Thanks

Luigi

2014-11-26 17:36 GMT+01:00 Charles Pick notifications@github.com:

@luigidellaquila https://github.com/luigidellaquila why differentiate?
either always envelope or never envelope but alternating between the
two makes things really painful for consumers.


Reply to this email directly or view it on GitHub
#3051 (comment)
.

@phpnode
Copy link
Contributor

phpnode commented Nov 26, 2014

@luigidellaquila think your last example should also be enveloped, e.g.

{
  "result": [
      {
          "@type": "d",
          "@rid": "#5:0",
          "@version": 2,
          "@class": "OUser",
          "name": "admin",
          "password": "...",
          "status": "ACTIVE",
          "roles": [
              "#4:0"
          ],
          "@fieldTypes": "roles=n"
      },
      {
          "@type": "d",
          "@rid": "#5:1",
          "@version": 1,
          "@class": "OUser",
          "name": "reader",
          "password":
"{SHA-256}3D0941964AA3EBDCB00CCEF58B1BB399F9F898465E9886D5AEC7F31090A0FB30",
          "status": "ACTIVE",
          "roles": [
              "#4:1"
          ],
          "@fieldTypes": "roles=n"
      }
  ]
}

Agree it might break but can be linked to the version of the binary protocol being used?

@luigidellaquila
Copy link
Member

@phpnode you are right, just missed a cut'n'paste from Studio (it doesn't show the envelope in the function result...)

@luigidellaquila
Copy link
Member

about binary compatibility, I think we cannot rely on binary protocol because it's not involved, here we are using HTTP/REST interface and it doesn't send info about protocol version.
We should introduce an http protocol version in the REST call header, but at this step it's quite tricky, not at development level but for clients. If you want we can also talk about this...

@mattaylor
Copy link
Contributor

Luigi. I like your proposal. Less is more as It should be easy enough to modify results to cobform to the old style.
However are there any constraints on the response format to allow these functions to be used within sql queries?

@mattaylor
Copy link
Contributor

The argument for selectively wrapping the resukts is that we can ensure the response should always include a results property which is an array of objects. This is quite helpfull

@phpnode
Copy link
Contributor

phpnode commented Nov 27, 2014

@mattaylor not sure I understand your last point, could you explain a bit more please?

@luigidellaquila I've said this before in #2273 and #2243 but there are some fundamental problems with the REST/JSON api anyway, it would be really good to be able to fix these problems by adopting a new format (could be opt in).

@mattaylor
Copy link
Contributor

My point is that it is helpfull for clients of the api to know that function calls should always return a consitent data schema. Ie a results array of objects. If the 'results' property of the response object is sometimes a scalar, sometimes an array and sometimes in object then any tooling around the api becomes much harder inc validators, sdk generators, docs, etc..

@phpnode
Copy link
Contributor

phpnode commented Nov 27, 2014

@mattaylor isn't that the situation we're already in?

@mattaylor
Copy link
Contributor

No - current implmentation is still inconsitent in that arrays are returned wrapped in a 'value' param. Ie

return [{a:1}] 
=> { "result":  [  {  "@type": "d",  "@version": 0, "value": [ {  "a": 1 } ] } ]

It would be more consitent to just return

=> { "result":  [  {  "a": 1  } ] }

ie always return a result array of objects

Luigis proposal is also ok with me as it allows developers to finer control of the response.

@mattaylor
Copy link
Contributor

Please can we get this reopened or a new ticket created to implement luigis proposals.

@lvca lvca reopened this Feb 5, 2015
@lvca lvca modified the milestones: 2.1, 2.0-RC1 (Release Candidate) Feb 5, 2015
@vertex-github
Copy link

I like the "same for all results" approach - keeping things consistent between results with regards to the envelope makes it easier for developers to understand/diagnose/inspect values.

@mattaylor
Copy link
Contributor

Luigi's proposal gives developers the option for implementing whatever response structure you like. The existing envelop approach does not and also creates major and un resolvable inconsitencies between functions which return db.queries and functions which return custom built lists.

@lvca lvca removed this from the 2.1-rc1 milestone Apr 1, 2015
@lvca lvca removed this from the 2.1-rc1 milestone Apr 1, 2015
@lvca lvca removed the bug label Apr 8, 2015
@mattaylor
Copy link
Contributor

In order to minimise back compatability issues, can we add a flag on Ofunction to signal how to serialise the function response. if this flag is missing then the old style is used, if the flag is there then the new style should be used. We could also havea global default properties as to whether new functions should use this flag by default, and also whether or not it should forced on existing functions as well.

@lvca lvca modified the milestones: 2.1-rc2, 2.1 GA May 5, 2015
@lvca lvca modified the milestones: 2.2, 2.1 GA Jul 3, 2015
@optimuspaul
Copy link

I was trying my hand at javascript functions in orient and let me just say I am majorly disappointed. I want to do something simple like load some results and inspect their properties. I can't figure out how to do that. It appears that if I look at a record in the JS function it's completely empty. However if I return it from the function it's all filled out with data, WTF? Also in the Studio testing functions that do things like queries across edges I don't get anything back, yet calling the same function with the same param on the rest api I get data, WTF? Consistency would be nice, but it seems you are consistent about one thing, being inconsistent.

@lvca
Copy link
Member

lvca commented Sep 2, 2015

@optimuspaul it's hard to help you without any example... Could you provide what is working via REST and not in Studio? Something I can copy & paste on an empty db.

@optimuspaul
Copy link

@lvca of course you are right. Here is an example:
I have two vertex classes, eos_object and eos_fingerprint.

make some stuff

create vertex eos_object content {"uid": "2424"}
create vertex eos_fingerprint content {"fingerprint": "aaa"}
create edge E from #12:4 to #13:5  --- of course make those id's match yours.

I have a function object_ping, with one parameter fingerprint

var gdb = orient.getGraph();
try {
  return gdb.command("sql", 'select uid, @rid as rid, out(eos_fingerprint).@rid as out_rid, out(eos_fingerprint).fingerprint as fingerprint from eos_object where out(eos_fingerprint)[fingerprint] IN ?', [fingerprint]);
} catch(e) {
  return {"status": "ERROR", "error": e.toString()};
}

in the studio, on the function page, when I test the function with "aaa" as the fingerprint to check I get 0 results. But on the rest API I get:

{
"result": [
{
"@type": "d",
"@rid": "#-2:0",
"@version": 0,
"uid": "2424",
"rid": "#12:4",
"out_rid": [
"#13:5"
],
"fingerprint": [
"aaa"
],
"@fieldTypes": "rid=x,out_rid=z"
}
]
}

When I run select object_ping("aaa") in the browse tab I do get:

[{"@type":"d","@rid":"#-2:0","@version":0,"uid":"2424","rid":"#12:4","out_rid":["#13:5"],"fingerprint":["aaa"],"@fieldTypes":"rid=x,out_rid=z"}]

Which does't seem quite right, is that json? Why didn't it make it look like other results?

@lvca lvca added the bug label Sep 23, 2015
@lvca lvca modified the milestones: 2.2.0-beta, 2.2.0-rc1 Dec 13, 2015
@lvca lvca modified the milestones: 2.2.0-rc1, 2.2.0 GA May 7, 2016
@robfrank robfrank modified the milestones: 2.2.0 GA, 2.2.1 May 18, 2016
@robfrank robfrank modified the milestones: 2.2.1, 2.2.x (next hotfix) Jun 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

9 participants