Solr
  1. Solr
  2. SOLR-1592

Refactor XMLWriter startTag to allow arbitrary attributes to be written

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 1.5, 3.1, 4.0-ALPHA
    • Component/s: None
    • Labels:
      None
    • Environment:

      My MacBook laptop.

      Description

      There are certain cases in which a user would like to write arbitrary attributes as part of the XML output for a field tag. Case in point: I'd like to declare tags in the SOLR output that are e.g., georss namespace, like georss:point. Other users may want to declare myns:mytag tags, which should be perfectly legal as SOLR goes. This isn't currently possible with the XMLWriter implementation, which curiously only allows the attribute "name" to be included in the XML tags.

      Coincidentally, users of XMLWriter aren't allowed to modify the <response outer XML tag to include those arbitrary namespaces (which was my original thought as a workaround for this). This wouldn't matter anyways, because by the time the user got to the FieldType#writeXML method, the header for the XML would have been written anyways.

      I've developed a workaround, and in doing so, allowed something that should have probably been allowed in the first place: allow a user to write arbitrary attributes (including xmlns:myns="myuri") as part of the XMLWriter#startTag function. I've kept the existing #startTag, but replaced its innards with versions of startTag that include startTagWithNamespaces, and startTagNoAttrs.

      1. SOLR-1592.Mattmann.112209_02.patch.txt
        4 kB
        Chris A. Mattmann
      2. SOLR-1592.Mattmann.112209.patch.txt
        3 kB
        Chris A. Mattmann
      3. SOLR-1592.patch
        2 kB
        Noble Paul
      4. SOLR-1592.patch
        0.9 kB
        Noble Paul

        Issue Links

          Activity

          Hide
          Chris A. Mattmann added a comment -

          here's a cleaner version of the patch with a few more methods. I can add javadocs to them to explain them better, but I think this is a lot more flexible than the current methods that exist (see the patch I'm about to attach to SOLR-1586 as evidence).

          Show
          Chris A. Mattmann added a comment - here's a cleaner version of the patch with a few more methods. I can add javadocs to them to explain them better, but I think this is a lot more flexible than the current methods that exist (see the patch I'm about to attach to SOLR-1586 as evidence).
          Hide
          Noble Paul added a comment -

          isn't this good enough?

          Show
          Noble Paul added a comment - isn't this good enough?
          Hide
          Noble Paul added a comment -

          added a method to write cdata also

          Show
          Noble Paul added a comment - added a method to write cdata also
          Hide
          Chris A. Mattmann added a comment -

          Hey Noble:

          I like your latest patch. My +1 on it – it meets my use case (and what I'm doing with SOLR-1586). I'll attach a new patch for SOLR-1586 that uses this code if you are OK to commit it.

          Cheers,
          Chris

          Show
          Chris A. Mattmann added a comment - Hey Noble: I like your latest patch. My +1 on it – it meets my use case (and what I'm doing with SOLR-1586 ). I'll attach a new patch for SOLR-1586 that uses this code if you are OK to commit it. Cheers, Chris
          Hide
          Noble Paul added a comment -

          committed r884411
          Thanks Mattman

          Show
          Noble Paul added a comment - committed r884411 Thanks Mattman
          Hide
          Chris A. Mattmann added a comment -

          Thanks, Noble! This works for me. +1 to resolve...

          I'll attach a patch to SOLR-1586 that leverages this new code...

          Show
          Chris A. Mattmann added a comment - Thanks, Noble! This works for me. +1 to resolve... I'll attach a patch to SOLR-1586 that leverages this new code...
          Hide
          Lance Norskog added a comment -

          Please add unit tests and test against the XSLTResponseWriter. In particular please check namespace handling with XSL.

          Do the other responsewriters care about attributes?

          Show
          Lance Norskog added a comment - Please add unit tests and test against the XSLTResponseWriter. In particular please check namespace handling with XSL. Do the other responsewriters care about attributes?
          Hide
          Noble Paul added a comment -

          lease add unit tests and test against the XSLTResponseWriter. ?

          What has this got to do with XSLTResponseWriter? The methods added are like helper methods which can be used by custom response writers.

          Show
          Noble Paul added a comment - lease add unit tests and test against the XSLTResponseWriter. ? What has this got to do with XSLTResponseWriter? The methods added are like helper methods which can be used by custom response writers.
          Hide
          Chris A. Mattmann added a comment -

          Hi Lance:

          I'd have to agree with Noble on this – the methods that were added were simple helper methods – we didn't change any core code nor did we expose any features that needed regression checks?

          Cheers,
          Chris

          Show
          Chris A. Mattmann added a comment - Hi Lance: I'd have to agree with Noble on this – the methods that were added were simple helper methods – we didn't change any core code nor did we expose any features that needed regression checks? Cheers, Chris
          Hide
          Lance Norskog added a comment -

          Ok, never mind.

          Show
          Lance Norskog added a comment - Ok, never mind.
          Hide
          Hoss Man added a comment -

          Correcting Fix Version based on CHANGES.txt, see this thread for more details...

          http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E

          Show
          Hoss Man added a comment - Correcting Fix Version based on CHANGES.txt, see this thread for more details... http://mail-archives.apache.org/mod_mbox/lucene-dev/201005.mbox/%3Calpine.DEB.1.10.1005251052040.24672@radix.cryptio.net%3E
          Hide
          Grant Ingersoll added a comment -

          Bulk close for 3.1.0 release

          Show
          Grant Ingersoll added a comment - Bulk close for 3.1.0 release

            People

            • Assignee:
              Noble Paul
              Reporter:
              Chris A. Mattmann
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development