Bug 432907 (bz-json)

Create a JSON frontend for WebServices

RESOLVED FIXED in Bugzilla 3.6

Status

()

enhancement
P1
normal
RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: mkanat, Assigned: mkanat)

Tracking

3.1.4
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +

Details

(Whiteboard: [roadmap: 4.0])

Attachments

(1 attachment, 7 obsolete attachments)

Assignee

Description

11 years ago
Instead of having a whole separate JSON API, I want to create multiple frontends for the WebService interface we already have. That way we could have all sorts of different methods of exposing the same API.

The first new frontend will be a JSON frontend.
Assignee

Updated

11 years ago
Whiteboard: [roadmap: 4.0]
Assignee

Updated

11 years ago
Priority: -- → P1
Assignee

Comment 1

11 years ago
We're going to use JSON-RPC for this, I think, which is handy since it's modeled to be a JSON version of XML-RPC.
Assignee

Comment 2

11 years ago
For methods that take simple arguments, we could also have the following call convention:

json.cgi?method=Bug.get&ids=1

And that would just return the JSON result.

Of course, anything complex (such as arrays within hashes or hashes within hashes) would require a formal JSON-RPC call.
Assignee

Comment 3

11 years ago
Posted patch Work In Progress (obsolete) — Splinter Review
This is a functional JSON interface for WebServices. It doesn't handle things like error codes or login yet, though. The only part of the WebService I've currently made functional is WebService::Bugzilla.

I've made it so that you can call methods like this:

 json.cgi?_method=Bugzilla.timezone

In addition to the normal method of doing JSON-RPC. That will return something that looks like this:

 {"version":"1.1","result":{"timezone":"-0700"}}
Assignee

Updated

11 years ago
Status: NEW → ASSIGNED
Assignee

Comment 4

11 years ago
This is another work-in-progress patch. This one is fully functional (it handles logins, throws errors correctly, etc.), it just needs some code cleanup.

I'm not thrilled with having to set Bugzilla->_json_server in order to throw errors, but given the current architecture of JSON::RPC, I don't have too many other choices.

There's a part of this patch that can be split out into another bug, which is the part that changes "type()" to "$self->type()" everywhere.
Attachment #323955 - Attachment is obsolete: true
Assignee

Updated

11 years ago
Depends on: 437617
Assignee

Comment 5

11 years ago
Comment on attachment 324034 [details] [diff] [review]
Work In Progress, v2 (Wrong patch)

I attached the wrong patch, will attach a good one.
Attachment #324034 - Attachment description: Work In Progress, v2 (Fully Functional) → Work In Progress, v2 (Wrong patch)
Attachment #324034 - Attachment is obsolete: true
Assignee

Comment 6

11 years ago
Here's the actual v2 Work In Progress
Assignee

Comment 7

11 years ago
Posted patch WIP v3 (obsolete) — Splinter Review
This is pretty much ready for review. Of course, we're waiting on the blocker to be reviewed first.

I've fixed up the code structure so that there isn't any code duplication between XML-RPC and JSON, but it required some weird, kind of hacky stuff.

I haven't fixed up the POD anywhere, yet.

Also, I think you still can't call JSON methods with the standard POST call method, and I have to fix that up. Shouldn't be too hard...or it might be impossible! :-) I guess we'll see.
Attachment #324039 - Attachment is obsolete: true
Assignee

Comment 8

11 years ago
So, there's two incompatible versions of JSON-RPC (three really, but we're not going to use JSON-RPC 1.0): 1.1, and 2.0.

2.0 is apparently "simpler", but has no real server or client implementations in existence. It also apparently requires an "id" argument with every call, which seems annoying to me, given how we're going to be using it.

1.1 is apparently more complex, but is what the Perl JSON-RPC module supports.

So I dunnow, are we going to be stuck with an obsolete standard if we go with 1.1? But 2.0...implementing it would probably add complexity to Bugzilla. And we'd have to use JSON::RPC::Common, which depends on Moose (which is too slow to compile to use in mod_cgi).

I think if JSON::RPC::Common used Mouse (a lightweight Moose), this would be an easier decision.
Assignee

Updated

11 years ago
Alias: bz-json
Assignee

Updated

11 years ago
Depends on: 475151
Assignee

Comment 9

11 years ago
Posted patch WIP v4 (obsolete) — Splinter Review
Okay, here's a more up-to-date work-in-progress. Note that this requires the patch on the blocker.

This is a fully-functional implementation that can read from POST.

I still need to block GET requests, because those expose us to CSRF attacks. Eventually we can selectively allow certain things to be requested over GET (any function that doesn't change things).

I also still need to write up and move around a lot of documentation.

I'm a little unhappy with how we're representing dates--look for the FIXME comment in the patch. If anybody has any suggestions there, please let me know.

I still need to parse dates on the way in, too, which is slightly tricky since I don't know which fields are supposed to be dates (unlike XML-RPC, which tells us explicitly).
Attachment #324109 - Attachment is obsolete: true
Assignee

Comment 10

11 years ago
Posted patch v5 (obsolete) — Splinter Review
Okay, I think everything strange about this should be explained well in the README, the POD, or the code comments. Here we go!
Attachment #358603 - Attachment is obsolete: true
Attachment #359598 - Flags: review?(wurblzap)
Attachment #359598 - Flags: review?(wurblzap) → review?
Comment on attachment 359598 [details] [diff] [review]
v5

I'm completely bereft of knowledge when it comes to JSON. All I know is what the abbreviation means, and this is just because I looked it up this very minute :)
Assignee

Updated

11 years ago
Attachment #359598 - Flags: review? → review?(dkl)
Comment on attachment 359598 [details] [diff] [review]
v5


>=== modified file 'Bugzilla/Constants.pm'
>-    USAGE_MODE_WEBSERVICE
>+    USAGE_MODE_XMLRPC

You need to change xmlrpc.cgi to use the new usage mode name or
make the needed changes in the XMLRPC refactoring bug 475151.
t/001compile.t fails on xmlrpc.cgi due to this even with the latest
patch on bug 475151 also applied.

>     USAGE_MODE_EMAIL
>+    USAGE_MODE_JSON
> 
>     ERROR_MODE_WEBPAGE
>     ERROR_MODE_DIE
>     ERROR_MODE_DIE_SOAP_FAULT
>+    ERROR_MODE_JSON_RPC

Question: Should we go ahead and change ERROR_MODE_DIE_SOAP_FAULT to ERROR_MODE_DIE_XMLRPC_FAULT
as we try to move away from SOAP::Lite in the future? Can be a separate bug later.

>=== modified file 'Bugzilla/Object.pm'
>     my $dbh = Bugzilla->dbh;
>+trick_taint($_) foreach @$values;
>     my $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @$values);
>     bless ($_, $class) foreach @$objects;
>     return $objects

Is this supposed to be here? If yes, then need to indent.

>=== modified file 'Bugzilla/WebService/Constants.pm'
>--- Bugzilla/WebService/Constants.pm	2009-01-24 11:53:21 +0000
>+++ Bugzilla/WebService/Constants.pm	2009-01-24 12:04:30 +0000
>@@ -133,5 +133,4 @@
>     return $dispatch;
> };
> 
>-
> 1;

Nit: remove the extra line

>=== added file 'Bugzilla/WebService/Server/JSONRPC.pm
>+sub cgi { return Bugzilla->cgi; }

Is this required to override the JSON library in some way? If not can we just
access Bugzilla->cgi directly when needed?

>=== modified file 'Bugzilla/WebService/Server/XMLRPC.pm'
>+__END__
>+
>+=head1 NAME
>+
>+Bugzilla::WebService::Server::XMLRPC - The XML-RPC Interface to Bugzilla
>+

[snip]

This should really be part of bug 475151 as is not related to JSON-RPC.

>=== added file 'jsonrpc.cgi'
>+# This eval allows runtests to pass even if JSON::RPC isn't
>+# installed.
>+eval { require Bugzilla::WebService::Server::JSONRPC; };
>+

Please add ThrowCodeError('json_rpc_not_installed') error
here similar to xmlrpc.cgi.

Now onto actual testing.
Dave
Attachment #359598 - Flags: review?(dkl) → review-
After some experimenting one way I was able to get blessed objects to be respresented in JSON format properly was to add the following code right before the new() method in Bugzilla::WebService::Server::JSON:

# Hack as JSON::RPC uses JSON without the -convert_bless_universally
# import tag. Without this for some reason even though we set 
# allow_blessed(1) and convert_blessed(1), JSON::RPC is still returning
# blessed objects as undef.
eval q|  
    require B;
    *UNIVERSAL::TO_JSON = sub {
        my $b_obj = B::svref_2object( $_[0] );
        return    $b_obj->isa('B::HV') ? { %{ $_[0] } }
            : $b_obj->isa('B::AV') ? [ @{ $_[0] } ]
            : undef;
    }
|;

Max, you can add this now or we can further research a solution in another bug post-commit.

Dave
Assignee

Comment 14

11 years ago
Posted patch v6 (obsolete) — Splinter Review
Okay, this addresses all your comments.
Attachment #359598 - Attachment is obsolete: true
Attachment #361845 - Flags: review?(dkl)
Assignee

Comment 15

11 years ago
Oh, and we'll address the blessed thing in a separate bug.
Attachment #361845 - Flags: review?(dkl) → review-
Comment on attachment 361845 [details] [diff] [review]
v6

>Index: Bugzilla/Object.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v
>retrieving revision 1.30
>diff -u -r1.30 Object.pm
>--- Bugzilla/Object.pm	23 Jan 2009 03:27:45 -0000	1.30
>+++ Bugzilla/Object.pm	11 Feb 2009 22:16:06 -0000
>@@ -219,7 +219,12 @@
>     $sql .= " $postamble" if $postamble;
>         
>     my $dbh = Bugzilla->dbh;
>-    my $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @$values);
>+    # Sometimes the values are tainted, but we don't want to untaint them
>+    # for the caller. So we copy the array. It's safe to untaint because
>+    # they're only used in placeholders here.
>+    my @untainted = @$values;
>+    trick_taint($_) foreach @untainted;
>+    my $objects = $dbh->selectall_arrayref($sql, {Slice=>{}}, @untainted);
>     bless ($_, $class) foreach @$objects;
>     return $objects
> }

This is causing the following error when loading a bug in the UI:

undef error - Can't use an undefined value as an ARRAY reference at /var/www/html/bugzilla/Bugzilla/Object.pm line 225. 

It could be something screwy with my test instance but I checked everything I knew to check. When I backed
out the changes for Bugzilla/Object.pm only, everything worked properly again.

Dave
Dave
Assignee

Updated

11 years ago
Target Milestone: Bugzilla 4.0 → Bugzilla 3.6
Assignee

Comment 17

11 years ago
Posted patch v7Splinter Review
Okay, fixed that error. No idea why it wasn't happening with the previous code, since get_all calls _do_list_select with an undef $values always.
Attachment #361845 - Attachment is obsolete: true
Attachment #362100 - Flags: review?(dkl)
Comment on attachment 362100 [details] [diff] [review]
v7

Couple issues that should be addressed before final release:

1. Error messages are not properly displayed when running under mod_perl.
2. Blessed objects not properly converted into JSON (read Bug.pm).

Other than that the JSON API works as expected for the test cases I executed.
r=dkl
Attachment #362100 - Flags: review?(dkl) → review+
Assignee

Comment 19

11 years ago
Holding approval until we actually want to start checking things into trunk.
Flags: approval?
Assignee

Comment 20

10 years ago
I filed bug 486073 and bug 486074 for the follow-up issues.

Checking in Bugzilla.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla.pm,v  <--  Bugzilla.pm
new revision: 1.74; previous revision: 1.73
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/jsonrpc.cgi,v
done
Checking in jsonrpc.cgi;
/cvsroot/mozilla/webtools/bugzilla/jsonrpc.cgi,v  <--  jsonrpc.cgi
initial revision: 1.1
done
Checking in xmlrpc.cgi;
/cvsroot/mozilla/webtools/bugzilla/xmlrpc.cgi,v  <--  xmlrpc.cgi
new revision: 1.12; previous revision: 1.11
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.44; previous revision: 1.43
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.111; previous revision: 1.110
done
Checking in Bugzilla/Error.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Error.pm,v  <--  Error.pm
new revision: 1.25; previous revision: 1.24
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.31; previous revision: 1.30
done
Checking in Bugzilla/WebService.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService.pm,v  <--  WebService.pm
new revision: 1.18; previous revision: 1.17
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.61; previous revision: 1.60
done
Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.34; previous revision: 1.33
done
Checking in Bugzilla/WebService/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm,v  <--  Constants.pm
new revision: 1.25; previous revision: 1.24
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/README,v
done
Checking in Bugzilla/WebService/README;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/README,v  <--  README
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm,v
done
Checking in Bugzilla/WebService/Server/JSONRPC.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm,v  <--  JSONRPC.pm
initial revision: 1.1
done
Checking in Bugzilla/WebService/Server/XMLRPC.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Server/XMLRPC.pm,v  <--  XMLRPC.pm
new revision: 1.2; previous revision: 1.1
done
Checking in extensions/example/lib/WSExample.pm;
/cvsroot/mozilla/webtools/bugzilla/extensions/example/lib/WSExample.pm,v  <--  WSExample.pm
new revision: 1.4; previous revision: 1.3
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.110; previous revision: 1.109
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.277; previous revision: 1.276
done
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: approval? → approval+
Resolution: --- → FIXED
Assignee

Updated

10 years ago
Keywords: relnote

Updated

10 years ago
Blocks: 487342
Blocks: 490673
Assignee

Comment 21

10 years ago
Added to the release notes in bug 547466.
Keywords: relnote

Updated

8 years ago
You need to log in before you can comment on or make changes to this bug.