[OPEN-ILS-DEV] osrf_message.[ch]

[Scott McKellar]

I have some miscellaneous observations and suggestions about the
osrfMessage machinery.

--------------

An osrfMessage has a member full_param_string.  We never do anything
with it, except to initialize it to NULL.  We don't even bother to free
it.  Can we get rid of it?  Or does it have a future that just hasn't
materialized yet?

---------------

Another member, result_string, is almost as useless.  It stores a pointer
to an unparsed JSON string.  Immediately after populating it, we parse
the JSON string and store a pointer to the resulting jsonObject.  We
never refer to result_string again, except to free it when we destroy
the osrfMessage.

The only reason I can think of for storing result_string is that it
might sometimes be useful to look at in the course of an interactive 
debugging session.  Otherwise it's a waste of a malloc and free.

In at least one case (in osrf_message_deserialize()) we create the
jsonObject without storing a copy of the corresponding JSON string.
So result_string isn't even reliably populated.

---------------

Immediately above the definition of osrf_message_add_param() there's a
comment:

    /* only works if parse_json_params is false */

Since there is no identifier named "parse_json_params" anywhere in the
code base, this comment no longer makes whatever sense it ever did.
Should we eliminate it, or change it to something meaningful?

---------------

I found another deprecated identifier: osrf_message_free.  Since no
other source file is calling it any more, I'll get rid of it one of
these days, in favor of osrfMessageFree().

---------------

The function osrfMessageToJSON has a misleading name.  It doesn't
produce JSON text, it produces a jsonObject.  I suggest renaming it
to osrfMessageToObject.

Since nothing calls this function outside of osrf_message.c, I further
suggest making it a static function.

---------------

The function osrf_message_add_object_param() is never called from
anywhere.  Can we get rid of it?

If we don't get rid of it, maybe we can speed it up a bit.

This function receives a pointer to a jsonObject, translates it into
a JSON string, and then immediately parses the JSON string to create
another jsonObject.  When I saw this, I had several thoughts in 
succession:

1. That's silly, why not just call osrfObjectClone()?

2. No, that wouldn't yield the same results.  When we translate it to
a JSON string, we expand the classnames into extra layers of jsonObject.
jsonObjectClone() doesn't do that.

3. Actually it would probably be okay to clone.  Whoever gets the
resulting jsonObject will almost certainly create a JSON string from it
sooner or later, expanding the classnames in the process.  (At least
they would if they ever called the function.)  By deferring the
expansion, we save a few rounds through malloc and free.

4. Even if we can't clone for some reason, we could get the same effect
more directly by calling jsonObjectEncodeClass() to make an expanded
copy of the original jsonObject.  There's no need to incur the overhead
of a round trip through JSON text.

-------------

The above considerations become moot if we discard the 
osrf_message_add_object_param function altogether.  However they
still apply to several other spots where we serialize and immediately 
deserialize a jsonObject:

-- In osrfMessageToJSON(), for REQUEST messages
-- In osrfMessageToJSON(), for RESULT messages
-- In osrf_message_deserialize(), for the "params" element
-- In osrf_message_deserialize(), for the "content" element

I haven't tried to track down the ultimate destinies of all these
jsonObjects, but I suspect that they all get serialized sooner or later,
with their classnames expanded. before they get anyplace where the 
classnames matter.

--------------

I can prepare patches for these things, but I wanted to consult with my
betters first.

Scott McKellar
http://home.swbell.net/mck9/ct/