jackrabbit-oak-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Angela Schreiber <anch...@adobe.com>
Subject Oak-API - some comments and suggestions [1]
Date Fri, 13 Apr 2012 12:29:06 GMT
hi

here the summary of some initial observations that i made while
looking at the oak-api today:

a) NodeState is never used in oak-jcr

    as far as i saw the NodeState interface is never used in the
    scope of oak-jcr in contrast to TransientNodeState that
    is omnipresent.

    basically i got the impression that TransientNodeState
    isn't really about transient JCR modification but in fact
    *is* the representation of the persisted MK node state on
    the oak-API. oak-api consumers would never need to access
    the mk-NodeState.

    Suggestion(s):

    - remove NodeState from oak-api
    - move to 'kernel' package or really use the corresponding
      interfaces from mk.model package
    - still rename TransientNodeState to (variants following)
      > NodeState
        in this case i would rename KernelNodeState to KernelNode
        as on mk level it is called Node again.
      > NodeData, NodeInfo, Node* (to distinguish from jcr Node)
        in this case NodeState would still refer to mk and
        the MK-API should use the term NodeState instead of Node
      > Apply the same pattern then to PropertyState


b) TransientState not extending from NodeState

    at a first glance i found that pretty confusing. with the comment
    made in a) that actually makes sense... but the name is confusing.

    in fact that TransientNodeState already looks much more like a
    JCR node than like a mk-NodeState as it refers to it's parent, has
    an editor (-> see below) etc.

    Suggestion(s):
    -> see above.


c) NodeStateEditor : general

    the name implies that it is a TransientNodeStateModifier
    but in the implementation it is a mixture of ChangeLog|Set and
    a ItemModified (not only nodes)... that looks like a confusion
    of intents to me.

    Suggestion(s):

    - at least rename NodeStateEditor to something like Item*Modifier
      or ...Editor where * would match whatever solution we come up
      with in point a).

    - if we made TransientNodeState the main node-like interface
      on the oak-api that is really distinct from the NodeState
      concept in mk and the core impl, i would like to suggest to
      move the (few) modifier methods to that interface.
      for the sake of an API that is easy to read and easy to use.

    - The editor was then not used to edit states but could be e.g.
      renamed to ChangeSet|Log (or similar). it's single
      purpose was then to collect the changes.

    - I don't know yet to which extend a consumer of the oak-api
      really need to get in touch with that changeset. but it felt
      natural to me to pass the changeset to the Connection#commit.
      ... but i would need to fiddle around with the code to get a
      clear picture.


d) NodeStateEditor#getBaseNodeState() NodeState

    this method is never used from a consumer of the oak-API.
    to me that is again (see also NodeState interface) an indication
    that we expose information that was in fact an implementation
    detail... it's only the oak-core implementation that needs to
    know that.

    Suggestion:
    - remove that method for the oak-api
    - use implementation specific method or somehow add mk.model package.


e) NodeStore, NodeStateDiff, ChildNodeEntry:

    similar to NodeState i am not sure if that those really needed and
    properly located in oak-api. so far i didn't find any usage in
    oak-api.

    Suggestion:
    - remove those classes from the oak-api until the API-consumer
      (oak-jcr) would really use it or if we know for sure where
      it will be used very soon.
    - use mk.model interfaces instead in the oak-core implementations
      that are in the kernel package.


f) PropertyState

    as stated at point a) this interface currently serves two
    purposes as it is used both in NodeState and TransientNodeState.
    that looks like a mistake to me in the light of the 2 node state
    interfaces not being connected by super-sub-interface relationship.

    Suggestion:
    - remove PropertyState as a notion of MK-property state from the
      oak-api
    - add a Property* interface where * would be according to the name
      of the oak-api naming convention for item/states being exposed
      by that api... right now the consistent name on oak-api probably
      was "TransientPropertyState" ;)


so, looking at the findings in this first set of comments, i have
the impression that there are right now a lot of unused interfaces
in oak-api... unless someone objects i would like to go ahead
and clean up the oak-api package as this will help us getting
a clearer picture of what we have and what is really missing.

for the findings that need more fundamental changes (e.g.the
editor/changeset discussion), i would like to get your feedback
on whether that was feasible/desirable (in the first place from
michael, that was working on this).

kind regards
angela














Mime
View raw message