Commons Digester
  1. Commons Digester
  2. DIGESTER-107

Provide ability to capture namespace snapshots

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: Nightly Builds
    • Fix Version/s: None
    • Labels:
      None

      Description

      Attached patch adds the capability to snapshot current namespaces at any point during parsing. This is useful, for example, in Commons SCXML where the value of an attribute can contain an XPath expression which needs to be evaluated later. The testcase in the patch captures namespace snapshots for each object created by the digester. Feedback welcome.

      As an aside, does anyone mind if I bring the Digester JIRA versions up to date? Thanks.

      1. DIGESTER-107-02.patch
        10 kB
        Rahul Akolkar
      2. DIGESTER-107-01.patch
        10 kB
        Rahul Akolkar
      3. DIGESTER-107.patch
        11 kB
        Rahul Akolkar

        Activity

        Hide
        Rahul Akolkar added a comment -

        Suggested improvement to capture namespace snapshots and related test case.

        Show
        Rahul Akolkar added a comment - Suggested improvement to capture namespace snapshots and related test case.
        Hide
        Simon Kitching added a comment -

        This looks pretty good to me. The only question I have is whether the new members of the Digester class should be private rather than protected. A protected member is effectively part of the public contract for the class, which restricts our ability to modify implementations later; I think I would personally prefer to see all the members made private, and only the new getCurrentNamespaces method be public. The Digester class is currently a mix of both approaches, but the more recent feature additions have used private members.

        I wonder whether cloning the map is necessary. Instead, can member currentNamespaces simply be an unmodifiable map, as a new instance is created if any namespace mappings change?

        Cheers,

        Simon

        Show
        Simon Kitching added a comment - This looks pretty good to me. The only question I have is whether the new members of the Digester class should be private rather than protected. A protected member is effectively part of the public contract for the class, which restricts our ability to modify implementations later; I think I would personally prefer to see all the members made private, and only the new getCurrentNamespaces method be public. The Digester class is currently a mix of both approaches, but the more recent feature additions have used private members. I wonder whether cloning the map is necessary. Instead, can member currentNamespaces simply be an unmodifiable map, as a new instance is created if any namespace mappings change? Cheers, Simon
        Hide
        Rahul Akolkar added a comment -

        Thanks for the feedback. WRT the earlier patch:

        • I had the new members protected since to achieve the "caching" effect of the currentNamespaces member, the implementation spilt out into other methods (start/endPrefixMapping() – not just getCurrentNamespaces()) and was tied at the hip to the namespaces member (for the dirty flag) and therefore leaving the new members private (when namespaces is protected) would possibly leave subclasses in a wicked predicament.
        • The modifiability of the map may be better left to the semantics of the object model that the document gets parsed into, and I presumed it may be useful to have a namespaces snapshot that one can work with as one pleases, depending on the usecase (actually, a new instance is not created in the earlier patch, the old one is cleared – but that could easily be changed).

        Moving forward, I wonder whether the caching has any noticeable benefit (I have no measurements either way) and another proposal would be to not add any new members at all, and let getCurrentNamespaces() do all the work each time. I'll attach such a patch in a few minutes. Let me know which one you'd prefer, thanks!

        Show
        Rahul Akolkar added a comment - Thanks for the feedback. WRT the earlier patch: I had the new members protected since to achieve the "caching" effect of the currentNamespaces member, the implementation spilt out into other methods (start/endPrefixMapping() – not just getCurrentNamespaces()) and was tied at the hip to the namespaces member (for the dirty flag) and therefore leaving the new members private (when namespaces is protected) would possibly leave subclasses in a wicked predicament. The modifiability of the map may be better left to the semantics of the object model that the document gets parsed into, and I presumed it may be useful to have a namespaces snapshot that one can work with as one pleases, depending on the usecase (actually, a new instance is not created in the earlier patch, the old one is cleared – but that could easily be changed). Moving forward, I wonder whether the caching has any noticeable benefit (I have no measurements either way) and another proposal would be to not add any new members at all, and let getCurrentNamespaces() do all the work each time. I'll attach such a patch in a few minutes. Let me know which one you'd prefer, thanks!
        Hide
        Rahul Akolkar added a comment -

        Patch that limits changes to getCurrentNamespaces() method. Test case is the same as the earlier patch.

        Show
        Rahul Akolkar added a comment - Patch that limits changes to getCurrentNamespaces() method. Test case is the same as the earlier patch.
        Hide
        Simon Kitching added a comment -

        Ok, let's go with the simplest patch (no caching).

        I don't think the code should be catching and ignoring Exception though (there aren't any checked exceptions thrown by the enclosed block are there?). It could either call createSaxException, or just let the original exception propagate. I think catching RuntimeException, logging it then rethrowing the original exception would be my preference, as this is such an odd case a checked exception doesn't seem worthwhile.

        Show
        Simon Kitching added a comment - Ok, let's go with the simplest patch (no caching). I don't think the code should be catching and ignoring Exception though (there aren't any checked exceptions thrown by the enclosed block are there?). It could either call createSaxException, or just let the original exception propagate. I think catching RuntimeException, logging it then rethrowing the original exception would be my preference, as this is such an odd case a checked exception doesn't seem worthwhile.
        Hide
        Rahul Akolkar added a comment -

        Correct, I should've checked the type heirarchy for ESE. In any case, a RuntimeException shouldn't happen if the Digester "namespaces" book-keeping goes according to plan. I'll attach another patch in a minute.

        Show
        Rahul Akolkar added a comment - Correct, I should've checked the type heirarchy for ESE. In any case, a RuntimeException shouldn't happen if the Digester "namespaces" book-keeping goes according to plan. I'll attach another patch in a minute.
        Hide
        Rahul Akolkar added a comment -

        Revised patch per earlier comment.

        Show
        Rahul Akolkar added a comment - Revised patch per earlier comment.
        Hide
        Simon Kitching added a comment -

        Looks good to me. I´d be happy for this to be committed.

        I guess you'll be asking about when a new Digester release is due next? It's probably a good time; a couple of minor bugfixes/enhancements have been made that deserve release (see RELEASE-NOTES.txt).

        I'm currently away on a trip (due back in two weeks) so can't offer to get anything done right now..

        Show
        Simon Kitching added a comment - Looks good to me. I´d be happy for this to be committed. I guess you'll be asking about when a new Digester release is due next? It's probably a good time; a couple of minor bugfixes/enhancements have been made that deserve release (see RELEASE-NOTES.txt). I'm currently away on a trip (due back in two weeks) so can't offer to get anything done right now..
        Hide
        Rahul Akolkar added a comment -

        Committed to trunk in r449882. Resolving as Fixed.

        Show
        Rahul Akolkar added a comment - Committed to trunk in r449882. Resolving as Fixed.

          People

          • Assignee:
            Unassigned
            Reporter:
            Rahul Akolkar
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development