Jackrabbit Content Repository
  1. Jackrabbit Content Repository
  2. JCR-433

NodeTypeRegistry could auto-subtype from nt:base

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.9, 1.0, 1.0.1
    • Fix Version/s: 1.1
    • Component/s: nodetype
    • Labels:
      None

      Description

      when tying to register a (primary) nodetype that does not extend from nt:base the following error is
      thrown:

      "all primary node types except nt:base itself must be (directly or indirectly) derived from nt:base"

      since the registry is able to detect this error, it would be easy to auto-subtype all nodetypes from nt:base. imo it's pointless to explzitely add the nt:base to every supperclass set. as an analogy, you don't need to 'extend from java.lang.Object' explicitely - the compiler does that automatically for your.

      1. jackrabbit-ntd.r418787.patch
        10 kB
        Jukka Zitting
      2. jackrabbit-ntr.r41878.patch
        2 kB
        Jukka Zitting
      3. jackrabbit-ntd-r41844.patch
        3 kB
        Jukka Zitting

        Activity

        Hide
        Jukka Zitting added a comment -

        Attached is a possible patch for fixing this.

        The patch modifies NodeTypeDef.getSupertypes() to return nt:base for all primary types with no explicitly set supertypes. This should cover not only CND, but all tools and code that work on node types.

        Show
        Jukka Zitting added a comment - Attached is a possible patch for fixing this. The patch modifies NodeTypeDef.getSupertypes() to return nt:base for all primary types with no explicitly set supertypes. This should cover not only CND, but all tools and code that work on node types.
        Hide
        Stefan Guggisberg added a comment -

        +1 for the proposed patch, thanks.

        Show
        Stefan Guggisberg added a comment - +1 for the proposed patch, thanks.
        Hide
        Tobias Bocanegra added a comment -

        i have concerns:

        • the static equals method might be very expensive
        • don't call equals(getSupertypes(), other.getSupertypes()), this is bad practice. rather call:
          NodeTypeDef.equals(getSupertypes(), other.getSupertypes()).

        are you sure this covers all cases correctly? if so, the nt:base errors in nodetype registry should be removed. i would prefer a proper nt:base check and automatic addition in the registry.

        Show
        Tobias Bocanegra added a comment - i have concerns: the static equals method might be very expensive don't call equals(getSupertypes(), other.getSupertypes()), this is bad practice. rather call: NodeTypeDef.equals(getSupertypes(), other.getSupertypes()). are you sure this covers all cases correctly? if so, the nt:base errors in nodetype registry should be removed. i would prefer a proper nt:base check and automatic addition in the registry.
        Hide
        Jukka Zitting added a comment -

        Tobias:
        > - the static equals method might be very expensive

        Only if the supertype arrays are large, not too likely in my mind. In any case also the existing supertype equality checks are expensive on large sets.

        > - don't call equals(getSupertypes(), other.getSupertypes()), this is bad practice. rather call:
        > NodeTypeDef.equals(getSupertypes(), other.getSupertypes()).

        Good point, thanks.

        > are you sure this covers all cases correctly? if so, the nt:base errors in nodetype registry should be removed.
        > i would prefer a proper nt:base check and automatic addition in the registry.

        Pretty much, yes. I first started with changing the nodetype registry, but it felt more natural to change NodeTypeDef. I'll make an alternative patch for just the nodetype registry for comparison.

        Show
        Jukka Zitting added a comment - Tobias: > - the static equals method might be very expensive Only if the supertype arrays are large, not too likely in my mind. In any case also the existing supertype equality checks are expensive on large sets. > - don't call equals(getSupertypes(), other.getSupertypes()), this is bad practice. rather call: > NodeTypeDef.equals(getSupertypes(), other.getSupertypes()). Good point, thanks. > are you sure this covers all cases correctly? if so, the nt:base errors in nodetype registry should be removed. > i would prefer a proper nt:base check and automatic addition in the registry. Pretty much, yes. I first started with changing the nodetype registry, but it felt more natural to change NodeTypeDef. I'll make an alternative patch for just the nodetype registry for comparison.
        Hide
        Jukka Zitting added a comment -

        Attached a patch (jackrabbit-ntr.r41878.patch) for fixing this by changing just NodeTypeRegistry.

        This is a bit problematic as the validateNodeTypeDef() method needs to modify the given NodeTypeDef to avoid having to add special case code all around the node type access code.

        Show
        Jukka Zitting added a comment - Attached a patch (jackrabbit-ntr.r41878.patch) for fixing this by changing just NodeTypeRegistry. This is a bit problematic as the validateNodeTypeDef() method needs to modify the given NodeTypeDef to avoid having to add special case code all around the node type access code.
        Hide
        Jukka Zitting added a comment -

        Attached an updated version (jackrabbit-ntd.r418787.patch) of the original NodeTypeDef patch.

        This patch streamlines the internal storage of the specified supertypes and removes a number of special cases that no longer apply (getSupertypes() never returns null, no need to explicitly add nt:base if not specified).

        Although this approach touches more code than just modifying the NodeTypeRegistry, this is IMO much cleaner from a design perspective (no need to modify the NodeTypeDef once it's been parsed, and better encapsulation of the default inheritance rule).

        Show
        Jukka Zitting added a comment - Attached an updated version (jackrabbit-ntd.r418787.patch) of the original NodeTypeDef patch. This patch streamlines the internal storage of the specified supertypes and removes a number of special cases that no longer apply (getSupertypes() never returns null, no need to explicitly add nt:base if not specified). Although this approach touches more code than just modifying the NodeTypeRegistry, this is IMO much cleaner from a design perspective (no need to modify the NodeTypeDef once it's been parsed, and better encapsulation of the default inheritance rule).
        Hide
        Stefan Guggisberg added a comment -

        Comment by Jukka Zitting
        > Attached an updated version (jackrabbit-ntd.r418787.patch) of the original NodeTypeDef patch.

        +1 for this patch

        Show
        Stefan Guggisberg added a comment - Comment by Jukka Zitting > Attached an updated version (jackrabbit-ntd.r418787.patch) of the original NodeTypeDef patch. +1 for this patch
        Hide
        Jukka Zitting added a comment -

        Patch applied in revision 423074.

        Show
        Jukka Zitting added a comment - Patch applied in revision 423074.

          People

          • Assignee:
            Jukka Zitting
            Reporter:
            Tobias Bocanegra
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development