Issue Details (XML | Word | Printable)

Key: XMLRPC-75
Type: New Feature New Feature
Status: Closed Closed
Resolution: Fixed
Priority: Major Major
Assignee: Jochen Wiedmann
Reporter: Walter Mundt
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
XML-RPC

Introspection Patch for XML-RPC, current to SVN 2005-12-29 (r359943)

Created: 30/Dec/05 09:28 AM   Updated: 05/Oct/06 09:00 PM
Return to search
Component/s: Source
Affects Version/s: unspecified
Fix Version/s: None

Time Tracking:
Not Specified

File Attachments:
  Size
Java Source File Licensed for inclusion in ASF works IntrospectionExample.java 2005-12-30 05:36 PM Walter Mundt 0.8 kB
Text File Licensed for inclusion in ASF works xmlrpc-2.0-beta-introspection.patch 2005-12-30 09:29 AM Walter Mundt 24 kB
File Licensed for inclusion in ASF works xmlrpc3-metadata.diff 2006-03-02 06:22 AM Walter Mundt 28 kB
File Licensed for inclusion in ASF works xmlrpc3-metadata.diff 2006-03-01 03:48 AM Walter Mundt 25 kB
File Licensed for inclusion in ASF works xmlrpc3-metadata.diff 2006-02-25 02:27 AM Walter Mundt 23 kB
File Licensed for inclusion in ASF works xmlrpc3-metadata.diff 2006-02-24 11:16 AM Walter Mundt 30 kB
File Licensed for inclusion in ASF works xmlrpc3-metadata.diff 2006-02-24 08:01 AM Walter Mundt 28 kB

Resolution Date: 21/Aug/06 12:25 AM


 Description  « Hide
I've taken Aaron Hamid's Introspection patch (which was in turn derived from the one here: http://xmlrpc-c.sourceforge.net/hacks/helma-xmlrpc-introspection.diff ) and basically rewritten it to work with the current XML-RPC library.

I tried to set it up so that it followed the existing coding style as much as possible, and would not break any existing applications. For example, when I needed a list of handlers supported by a particular mapping, I added a new interface XmlRpcListableHandlerMapping that provides this information, made the default handler implement it, and designed my code so that if it were not implemented by a handler in use, everything would work except the system.listMethods functionality that relies on it.

I hope you will be able to integrate this code into the next version of the library; please contact me with any questions or issues. I will be attaching the patch to this issue if I can so that the problem with his previous patch will not occur.

 All   Comments   Work Log   Change History   Subversion Commits      Sort Order: Ascending order - Click to sort in descending order
Walter Mundt added a comment - 30/Dec/05 11:08 AM
By the way, the fault codes I use in the patch are from here:
http://xmlrpc-epi.sourceforge.net/specs/rfc.fault_codes.php

If you don't like using the codes from that RFC, feel free to change them back to zeros; I don't much care either way.

Walter Mundt added a comment - 30/Dec/05 05:36 PM
I've deliberately left the API for enabling automatic intropection rather obtuse as of yet so that the community can decide how best to make it accessible from outside the library. However, here is how you currently have to do it, for those that want to use the patch as-is.

Jochen Wiedmann added a comment - 30/Dec/05 09:13 PM
I haven't had the time to carefully read through your patch. I do not know "Aaron Hamid's Introspection patch" not do I now the one from xmlrpc-c.sf.net.

To me, it currently looks like yet another handler mapping. I wouldn't refuse the idea to add a different handler mapping to the source tree. The handler mapping API was written specifically with different mappings in mind. Otoh, not every mapping must be a port of the XML-RPC core distribution.

What I currently would like to see, would be some sentences, that describe what the differences between the default handler mapping and your suggestions are. What advantages, what disadvantages, and so on.

Walter Mundt added a comment - 31/Dec/05 02:28 AM
Don't worry about the original patches then, as they're not terrribly relevant -- they're both around 3-4 years old at this point, thus this new attempt. However, this is not just "yet another handler mapping." It adds a new interface that custom handlers can implement to support introspection, as well as a new derived Invoker that implements this interface using Java reflection. It also adds a new ContextXmlRpcHandler which makes use of this interface to actually implement the method calls specified by the introspection API, and a function in the SystemHandler to enable those methods if it is so desired.

First, I will argue that this is a useful addition to the library. Second, I will argue that this code belongs in the core distribution.

For the first point:
While introspection is not part of the XML-RPC specification, it is a very useful feature for automatic discovery of services and proxy interface generation, among other things. This particular introspection interface is supported both by the PHP XML-RPC extension here: http://us3.php.net/manual/en/ref.xmlrpc.php and the standard Python XML-RPC server library (although the latter doesn't yet support methodSignature queries automatically). Supporting it here would mean that this library would behave more similarly to other XML-RPC implementations that are available; whether this is desirable in and of itself is debatable, but I think it is if there is no downside to the proposition.

For the second:
First of all, it's harmless for those who do not wish to take advantage of it. None of the code for actually calling a particular RPC method is changed, so the only possible performance change is (very) slightly larger code size. Second, as I mentioned above, having it bundled increases similarity with other XML-RPC implementations. Third, some changes, that make the introspection API easier to use, such as the new introspection-related methods on the SystemHandler and DefaultHandlerMapping class, and the addition of introspection support to the SystemHandler, could not be implemented as elegantly were the introspection code to be published and maintained separately.
Finally, if introspection is not merged into the main source tree, this patch will likely end up out-of-date and useless, just as the previous one did, resulting in people who need introspection support sticking with older versions of the library and being unable to contribute to the project's current code. On the other hand, if it is merged, then it should be fairly easy to see when it breaks and fix it as the changes that require it to be updated are made.

Jochen Wiedmann added a comment - 31/Dec/05 05:41 AM
From your comment, I conclude that

a) there are implementations in other languages, which do similar things and that
b) it is based on introspection

As for a), I still do miss a description what does happen. The links you mention above are much more generic and no specifications or something similar. As for b), the existing DefaultHandler is based on reflection itself.

In other words, I do not have the feeling to know more.


Walter Mundt added a comment - 31/Dec/05 06:52 AM
I am sorry if my descriptions are insufficient. I am not sure what you are saying in your last sentence, but in case you would like more information I will try to answer the issues you raise.

a) This is correct. Other languages to implement this sub-protocol. I did not link to the pages specifically concerned with this before; here are those links (in alphabetical order by language):
C/C++: http://xmlrpc-c.sourceforge.net/doc/libxmlrpc_server.html#systemmethod
.NET/C#: http://www.xml-rpc.net/faq/xmlrpcnetfaq.html#3.5
PHP: http://www.php.net/manual/en/function.xmlrpc-server-add-introspection-data.php
Python: http://docs.python.org/lib/simple-xmlrpc-servers.html (see register_introspection_functions) and http://docs.python.org/lib/serverproxy-objects.html

The second Python link provides a very good description of the three functions this patch is concerned with implementing.

b) Also correct. However, what it does with introspection is quite different; rather than using it to implement a simple named method call operation, it essentially translates remote introspection requests into local Java reflection API calls.

Jochen Wiedmann added a comment - 01/Jan/06 09:32 AM
I do now understand, that the "Introspection" stuff is more than a new handler mapping. Basically, it is providing metadata to the client.

I'll look through the code in the next week.

Jochen Wiedmann added a comment - 23/Feb/06 03:48 PM
Walter, are you interested in getting this into the 3.0 branch?

If so, let's do it in a few steps.

- First of all, you should propose necessary extensions for the interfaces handlermapping and handler.
  (Recommended: Create an interface like XmlRpcMetaDataHandlerMapping, which extends
  XmlRpcHandlerMapping.)
- Second: Make the existing classes implement your new interfaces.
- Third: Provide special handlers, which provide the requested metadata.


Walter Mundt added a comment - 24/Feb/06 08:01 AM
How's this patch? Note that this assumes the patch from XMLRPC-76 has already been applied.

Walter Mundt added a comment - 24/Feb/06 09:44 AM
Oops! My earlier comment is incorrect. The attached patch _includes_ the changes from XMLRPC-76, sorry.

Jochen Wiedmann added a comment - 24/Feb/06 11:55 PM
Walter, I have added the interfaces XmlRpcListableHandlerMapping, and XmlRpcMetaDataHandler. Please note:

- I have added some comments. They represent my thoughts. As such, they are questionable.
  Please read them carefully and verify, whether they meet your expectations.
- I have added methods getMethodSignatures() and getMethodHelp() to the handler mapping.
  Obviously, the will usually be implemented by simply calling the respective handlers
  methods. However, I find it thinkable, that a handler mapping will not do this.

If the above is fine for you, then I suggest the following steps to follow:

- Modify your XmlRpcSystemImpl to use the handler mapping methods.
  (Nice idea, btw, to implement it like this. Did you note, that it can easily be
  added to the property mappings by simply adding a property "system"?
  Dynamic addition isn't required.)
- Make the default handler mappings implement listable handler mapping.
- Same for the default handlers, which should implement metadada handler.




Walter Mundt added a comment - 25/Feb/06 02:27 AM
Okay, here is an updated patch which uses your interfaces. It also adds support in PropertyHandlerMapping for automatically passing the mapping to the constructor for any object specified in the property file. This could be used to subclass PHM and store additional information used by the various XML-RPC services.

Walter Mundt added a comment - 25/Feb/06 02:29 AM
Oh, the patch also adds method overloading support to AbstractReflectiveHandlerMapping, so that, for example, a class that implements both foo(String) and foo(String[]) for some operation can make both versions available via XML-RPC.

Jochen Wiedmann added a comment - 28/Feb/06 03:24 PM
Walter,

I apologize, but I am unhappy with the overloading.

While I accept, that it might be suitable in certain situations, I do not think that it should be the default. In particular, I do not think that the selection of the right method via reflection should occur with any method invocation. That's something we had in version 2 and have just successfully managed to get rid of.

IMO, the AbstractReflectiveHandlerMapping should support a single method per name only. There is no reason for not adding more complicated subclasses, if you like that.

Jochen

Walter Mundt added a comment - 01/Mar/06 03:48 AM
Alright, if you insist, here's a version that makes overloading support an optional (and disabled-by-default) feature.

Walter Mundt added a comment - 02/Mar/06 06:22 AM
Oops! Minor fix, forgot to adjust a couple of references to the constructors for PropertyHandlerMapping that I changed to support overloading as an optional feature.

Jochen Wiedmann added a comment - 04/Mar/06 07:15 AM
Walter, I have committed some changes, that make AbstractReflectiveXmlRpcHandlerMapping implement the XmlRpcMetaDataHandlerMapping interface. Please, note the following:

- The implementation ensures, that the various "system.*" methods return
  consistent result. In particular, a method returned by "system.listMethods"
  is always able to provide a meaningful result for "system.methodSignature"
  and "system.methodHelp".
- I did not implement overloading. My goal is to keep the base classes simple
  and clean. I am still open for subclasses, which allow overloading.
- The code is not checked. It is quite likely, that you'll find bugs.
- Keep documentation in mind. If you need a pointer to the apt format, see
  http://maven.apache.org/guides/mini/guide-apt-format.html

Jochen Wiedmann added a comment - 23/May/06 03:30 AM
Returning to the discussions on xmlrpc-user: Considering the fact, that overloading was possible with version 2, I find that I should drop my position. Consequently I have today added support for method overloading.

Walter, I'd really hope that you finish your good work by adding some docs.

Jochen Wiedmann added a comment - 21/Aug/06 12:25 AM
Closing, user has lost interest.