XPath API project includes abstract base classes that seem implementation specific

[GoogleCodeExporter]

Initial (eric) report:

In particular, all the classes in the package 
org.apache.gxml.xpath.v10.expressions named *Expr.java are part of the 
implementation of XPath, not its use.

In any case, we should look at all of these classes carefully, so that we start 
by exposing the minimum required.

Developer (aaletal) response:

I'd suggest, rather, that the hierarchy rooted at ConvertibleExpr is at issue, 
and the question is whether the convertibility ought to be somehow mandated.

It's all a matter of taste, I suppose. ConvertibleExpr makes it so that it is 
possible to cast from any expression type to any other, in the four XPath 1.0 
types. That could be expressed as an interface. David chose to express it as 
abstract base classes, so that behavior could be shared deeply, and counted 
upon. "API" != "Interface only"; the question of where to draw the line can be 
obscure.

In this case, removing the convertible expressions from API implies removing 
Converter (one level up) as well, or changing all of these concrete and 
abstract classes to interfaces, which, in my opinion, is wrong: we know, in the 
API, not only what the methods are, but exactly what the behavior is, and the 
API ought to be enforcing the rules of conversion from expression-type to 
expression-type.

Original issue reported on code.google.com by aale...@gmail.com on 12 Oct 2010 at 4:36

[GoogleCodeExporter]

Digging deeper, here, I think I can articulate a way forward here.

The chief problem lies in mixing up the complexity of being a client of the 
XPath implementation, and that of looking to extend the implementation by way 
of adding a method.

ConvertibleExpr has a bunch of subclasses, and none of them are needed by the 
remainder of the client API.  And further, the immediate subclasses, like 
ConvertibleBooleanExpr don't actually add new methods to the hierarchy, so the 
only thing they contribute to the API is the ability to call "instanceof" to 
determine the type of expression - except that nothing does that.

However, I note that in order to implement the "here()" function for XML 
Security, I did extend the ConvertibleNodeSetExpr.

So, the question is, do we make this divide between the "client" API, and the 
"extension" API?

That would involve changing the return type on the methods of ConvertibleExpr 
to be of type ConvertibleExpr.  This change doesn't cause any compilation 
errors.

Comments?

Original comment by eric%tib...@gtempaccount.com on 24 Jan 2011 at 11:43

[GoogleCodeExporter]

I still do not see a problem with these concrete classes appearing in the API.

Problem: the XPath specification specifies certain behaviors associated with 
the 'data types' which are permissibly returned from various expressions.  That 
is, each expression is said to return a specific type, but the specification 
further mandates that each of these types are 'convertible' (or better, can be 
coerced to) each other type.

Additional problem: the type system in XPath 1 is *not* extensible.  So, there 
are four possible return types, which means that if "expression" is an 
abstraction, then there should be four "expression" abstractions, each 
converting, with known (specified) behavior to other types.

The API does *not* need to contain only interfaces.  In this case, the fact 
that the behavior (convertibility between four, and only four distinct 'types' 
of expression) is specified at least *sanctions* the current design.

As currently designed, adding a new extension of ConvertibleExpr doesn't work 
the way the four built-ins do--you'd be able to convert the new to the existing 
four, but would not convert the existing four to new.  This is, in my opinion, 
correct.  This all relies on the combined behavior of ConvertibleExpr and 
Converter (which each subclass calls), and effectively enforces the XPath 1 
contract that there are four (not forty, not twenty-whatever) data types that 
an expression can natively return, and that there are standard rules for 
casting from one of the four to any of the other four.

But if you wanna extend the XPath 1 type system to support one of the nineteen 
time-and-date-related W3C XML Schema data types, you're out of luck.  This is a 
good thing.

It's not an unreservedly good thing, perhaps; the choice of double as the 
numeric representation in XPath 1.0 was ... perhaps not ideal.  But that *is* 
what the specification says; you cannot create XPath 1 extension functions that 
return something other than boolean, string, nodeset, or double.  Reducing 
these classes to interfaces is not correct.  Moving them out of the API is not 
correct.

I do not think that there is a bug here, and would not like to see any of the 
changes so far suggested implemented.  It does what it's s'posed to.  Ain't 
broke.  Don't fix.

Original comment by aale...@gmail.com on 25 Jan 2011 at 2:27

[GoogleCodeExporter]

To highlight the API question a little more clearly, the existing XPath 
packages are tightly coupled. the o.g.x.v10 package refers to constructs in 
o.g.x.v10.expressions, and vice-versa.  Same with the other packages.

ConvertibleExpression is just the most glaring of these couplings, because from 
the base v10 package, you're pulled into using the v10.expressions package.  
Looking further, the MergeNodeIterator is a public class, but seems to be used 
only by the non-public ComposeExpr class.

It looks like we have ever possible pairwise package dependency.

Before we bake the APIs, we should figure out the package breakdown such that 
clients with simple demands on the API need not worry about all of the packages.

Original comment by eric%tib...@gtempaccount.com on 25 Jan 2011 at 5:33

[GoogleCodeExporter]

Okay.  The last comment seems to change the summary, to me.

My interpretation of the original summary: there are concrete classes in the 
API that should not be there; they should be in the implementation.  My 
foregoing responses are all to that address; I believe that these concrete 
classes enforce specified behaviors (and prevent extensibility of types).

My interpretation of the summary as of the last comment: there is too much 
inter-package coupling.  That's a different issue, and almost certainly true, 
but calls for different solutions than moving concrete classes into the 
implementation, I think.

Note first: the original code was all in one package (a common pattern for the 
original primary developer, who had no objection to having hundreds of classes 
in a single package). So the division was 'artificial', made primarily to make 
learning the API less intimidating. But we could certainly reduce inter-package 
coupling by returning to that (single-package) style.  There are forty-one 
classes, less than 2350 LOC.  Or we could repackage, such that some packages 
are independent (but there would certainly be coupling between the entry point, 
probably in the base package, and all other packages, or so one would hope).  
The iterator-expression coupling, for instance, is a legacy of the perhaps 
over-hasty partitioning of the single super-package into multiple packages.  
The point of repackaging was to make it easier on people learning the API (for 
a counter-example: please report how many minutes or hours it takes to find the 
primary entry point in the schema parser ... if more than thirty minutes, 
perhaps it would be better to stop, though).

That is, we could remove circularities, although I am not entirely clear that 
reducing coupling in this way is ... well, useful, to be blunt.  Yes, all of 
the XPath 1.0 API packages are related to one another.  I'd rather not have 
them circling about, but I don't see too much danger in it, so long as the 
entry points are clear, and there's no circularity extending *beyond* the quite 
limited scope of the XPath API packages.  41/2336 according to find+wc-l.

Original comment by aale...@gmail.com on 25 Jan 2011 at 2:55

[GoogleCodeExporter]

I've started working on a proposal for this that, at least in my opinion, 
dramatically improves the API. Will be posting changes to a branch for 
discussion.

Original comment by eric%tib...@gtempaccount.com on 28 Feb 2011 at 5:22

• Changed state: Started

[GoogleCodeExporter]

I believe the branch for these changes is just about complete.

/branches/xpath-api-proposal

I've got one question pending related to this issue that adjusts the API:
http://groups.google.com/group/genxdm/browse_thread/thread/d874b05031996685

Other than that, anyone have concerns about what I've got?

I'm planning to merge back onto trunk later tomorrow, absent objections.

Original comment by eric%tib...@gtempaccount.com on 3 Mar 2011 at 3:03

• Added labels: Milestone-Release0.9

[GoogleCodeExporter]

Fixed at revision 159

Original comment by eric%tib...@gtempaccount.com on 10 Mar 2011 at 4:29

• Changed state: Fixed

[GoogleCodeExporter]

Original comment by eric%tib...@gtempaccount.com on 29 Apr 2011 at 11:16

• Added labels: Milestone-20110415
• Removed labels: Milestone-Release0.9