First let me state that this "how to pass escape characters to plugin parameters of type string from different configuration sources" problem applies to every other plugin, e.g., `AbstractJacksonLayout.Builder#endOfLine`. Hence, nothing specific to `JsonTemplateLayout`.
That said, I share your hesitation to include a relatively complex `unescape()` routine into the code base. Though having both `eventDelimiter` and `nullEventDelimiterEnabled` parameters while the same effect can exactly be created with just `eventDelimiter` feels like a code smell to me. What about just backporting `translateEscapes()` of Java 13? This will still fit the bill and can be removed in the future. On Tue, Jul 14, 2020 at 7:41 PM Ralph Goers <ralph.go...@dslextreme.com> wrote: > > Do we really need to do this? The only two delimiters that have ever been > requested are null and newline. Defaulting to newline and having an option to > use null seems fine to me. > > Ralph > > > On Jul 14, 2020, at 5:49 AM, Volkan Yazıcı <volkan.yaz...@gmail.com> wrote: > > > > Thinking more on this issue made me question the ideal way to pass > > Java string literals to plugins. > > > > I first gave Matt's suggestion a try, set eventDelimiter="�" in the > > XML and got the following error: > > > > ERROR StatusLogger Error parsing /home/vy/...Logging.xml > > org.xml.sax.SAXParseException; systemId: > > file:///home/vy/...Logging.xml; lineNumber: 29; columnNumber: 55; > > Character reference "&# > > at > > com.sun.org.apache.xerces.internal.parsers.DOMParser.parse(DOMParser.java:257) > > > > Next I tried passing escapes from a properties files instead. Setting > > eventDelimiter=\r\n\0 results in string "\r\n0". > > > > Hence, I am pretty confident that the user input needs to be escaped > > and I need to unescape it upon reception. There is an ad-hoc > > `unescapeJava()` method shared in a StackOverflow post[1]. The thread > > also references StringEscapeUtils.unescapeJava() (from commons) and > > String#translateEscapes() (Java 13+) alternatives along with their > > caveats. > > > > At this stage, I am inclined to pick the solution shared in > > StackOverflow[1]. Note that accepting escape characters will void the > > need to nullEventDelimiterEnabled flag. To sum it up, > > > > 1. May I have your blessing for using the StackOverflow snippet? > > 2. Is it okay with regards to licensing? > > > > [1] https://stackoverflow.com/a/4298836/1278899 > > > > On Sun, Jul 12, 2020 at 7:35 PM Ralph Goers <ralph.go...@dslextreme.com> > > wrote: > >> > >> Maybe. But it isn’t obvious and would have to be explicitly documented. > >> > >> Ralph > >> > >>> On Jul 12, 2020, at 8:09 AM, Matt Sicker <boa...@gmail.com> wrote: > >>> > >>> Wouldn’t that be � in XML? > >>> > >>> On Sat, Jul 11, 2020 at 16:26 Ralph Goers <ralph.go...@dslextreme.com> > >>> wrote: > >>> > >>>> I guess what you mean is that where you have $resolver: “message” that > >>>> would somehow be replaced by $resolver” “pattern”? That would make > >>>> sense. > >>>> I have no problem with the changes you are suggesting. However, you > >>>> should > >>>> also change the configuration shown in cloud.md and the log4j2.xml in > >>>> log4j-spring-cloud-config/log4j-spring-cloud-samples/log4j-spring-cloud-config-sample-server/src/main/config-repo > >>>> to match. > >>>> > >>>> Thanks for determining that it was XML parsing that was messing with the > >>>> “\0” value. That implies there would have been an XMLish way to bypass > >>>> that > >>>> but using an explicit attribute is just a lot easier. The GelfLayout > >>>> uses > >>>> “includeNullDelimitor”. You might consider using that name for > >>>> consistency. > >>>> > >>>> Ralph > >>>> > >>>>> On Jul 11, 2020, at 1:46 PM, Volkan Yazıcı <volkan.yaz...@gmail.com> > >>>> wrote: > >>>>> > >>>>> I can't believe how were we expecting somebody to pass a null > >>>>> character from an XML file! Long story short, it was the XML parser, > >>>>> doing the right thing, that is, converting XML attribute value "\0" to > >>>>> Java String "\\0". Okay, I am not gonna whine about this anymore. I > >>>>> have just pushed a commit adding `nullEventDelimiterEnabled` flag > >>>>> along with a unit test. (Ralph, I've replaced your > >>>>> `eventDelimiter="null"` handling as well.) > >>>>> > >>>>> Note that my question about reverting the changes to `MessageResolver` > >>>>> still stands. > >>>>> > >>>>> On Sat, Jul 11, 2020 at 8:37 PM Volkan Yazıcı <volkan.yaz...@gmail.com> > >>>> wrote: > >>>>>> > >>>>>> Hello Ralph! > >>>>>> > >>>>>> Sorry that it took me this long to reply to you back in detail, but I > >>>>>> have just found time to do so. Let me try to address the points you > >>>>>> raised: > >>>>>> > >>>>>>> The first problem I ran into is that additional fields don’t work. > >>>>>>> I don’t see any configurations that have them and the Builder doesn’t > >>>>>>> have annotations on the key and value fields so I suspect it just > >>>>>>> doesn’t work. > >>>>>> > >>>>>> Yes, the annotations... :facepalm: Good catch. > >>>>>> > >>>>>>> Why didn’t you just enhance KeyValuePair to add the type attribute? > >>>>>> > >>>>>> I was a little bit hesitant to do that, since `type` is mostly a > >>>>>> concern in the context of JTL. Would you rather recommend me to extend > >>>>>> `KeyValuePair` instead? > >>>>>> > >>>>>>> I thought you said you added the ability to format the message using > >>>>>>> a pattern, but I don’t see that in the documentation or in > >>>>>>> MessageResolver. Was that not included? > >>>>>> > >>>>>> Dang it! Indeed I forgot to add `PatternResolver` into the docs. (I > >>>>>> will talk about this again in the following lines.) > >>>>>> > >>>>>>> I have added the ability to format the message field using a pattern. > >>>>>> > >>>>>> Doh! I totally misinterpreted your request. I thought you wanted to be > >>>>>> able to *only* employ `PatternLayout` in JTL, hence I created > >>>>>> `PatternResolver`. Though what you asked for is to treat the message > >>>>>> as a pattern. (Looking back right now, your request was pretty clear, > >>>>>> it was me who was confused.) > >>>>>> > >>>>>> I have reviewed the changes and I want to share two remarks: > >>>>>> > >>>>>> 1. I've seen that you have used a thread-local. That functionality is > >>>>>> totally delegated to the `RecyclerFactory` employed, which can be > >>>>>> provided by the user. (I will change this accordingly.) > >>>>>> > >>>>>> 2. I've noticed `PatternResolver` is missing the `includeStacktrace` > >>>>>> flag. I will add it. > >>>>>> > >>>>>>> I have modified the code so that specifying `eventDelimiter=“null”` > >>>>>>> will insert a null at the end of the message as specifying > >>>>>>> eventDelimiter=“\0” or “\u000” does not work (It ends up being “\\0” > >>>>>>> or “\u000”. > >>>>>> > >>>>>> I am really puzzled about this. Let me explain it through the sources. > >>>> These > >>>>>> are the lines you have added: > >>>>>> > >>>>>> if (eventDelimiter != null && > >>>> eventDelimiter.equalsIgnoreCase("null")) { > >>>>>> stringBuilder.append('\0'); > >>>>>> } else { > >>>>>> stringBuilder.append(eventDelimiter); > >>>>>> } > >>>>>> > >>>>>> Nowhere in the code `eventDelimiter` is JSON quoted. I am totally > >>>>>> clueless how setting `eventDelimiter` to `\0` doesn't create the > >>>>>> effect you manually execute here. Further, if it doesn't, how come > >>>>>> `JsonTemplateLayoutTest#test_null_eventDelimiter()` test passes? I see > >>>>>> nothing else but to blame two other suspects: > >>>>>> > >>>>>> 1. Either setting the `eventDelimiter` property to `\0` causes the > >>>>>> value to get quoted. > >>>>>> > >>>>>> 2. Or there is something else going on while passing the value from > >>>>>> the layout to the appender. > >>>>>> > >>>>>> On the other hand, I am a little bit distant to creating special cases > >>>>>> for certain values (e.g., `null`) of `eventDelimiter`. What if the > >>>>>> user literally wants to dump `null` (literally!) after every event? > >>>>>> > >>>>>>> As I was implementing stuff I noticed you implemented stuff using > >>>>>>> lots of static methods. I am not really sure why since almost all of > >>>>>>> those methods are only invoked from an instance of the object. > >>>>>> > >>>>>> I generally choose the strictest modifier combination, unless there is > >>>>>> a reason not to do so. Following this principle, if a method/field > >>>>>> doesn't have any instance dependencies, I make it static. Am I doing > >>>>>> something wrong with this convention? > >>>>>> > >>>>>>> Also, I would like to be able to specify the pattern I use for the > >>>>>>> message in the log4j configuration so it is easier to change > >>>>>>> dynamically but I couldn’t figure out how to do that and get the > >>>>>>> pattern passed to the MessageResolver. > >>>>>> > >>>>>> Doesn't `PatternResolver` fit the bill here? > >>>>>> > >>>>>> Cheers! > >>>>>> > >>>>>> On Wed, Jul 8, 2020 at 7:42 PM Ralph Goers <ralph.go...@dslextreme.com> > >>>> wrote: > >>>>>>> > >>>>>>> A couple other things. As I was implementing stuff I noticed you > >>>> implemented stuff using lots of static methods. I am not really sure why > >>>> since almost all of those methods are only invoked from an instance of > >>>> the > >>>> object. I had to change that to get my enhancements to work. > >>>>>>> > >>>>>>> Also, I would like to be able to specify the pattern I use for the > >>>> message in the log4j configuration so it is easier to change dynamically > >>>> but I couldn’t figure out how to do that and get the pattern passed to > >>>> the > >>>> MessageResolver. > >>>>>>> > >>>>>>> Ralph > >>>>>>> > >>>>>>>> On Jul 8, 2020, at 7:16 AM, Volkan Yazıcı <volkan.yaz...@gmail.com> > >>>> wrote: > >>>>>>>> > >>>>>>>> Hey Ralph, > >>>>>>>> > >>>>>>>> Thanks so much for taking time, really appreciated. Unfortunately the > >>>>>>>> last few days I was pretty busy both at home and at work. I will > >>>>>>>> check > >>>>>>>> out your changes and reply back as soon as I find some time. > >>>>>>>> > >>>>>>>> Kind regards. > >>>>>>>> > >>>>>>>> On Wed, Jul 8, 2020 at 1:08 AM Ralph Goers < > >>>> ralph.go...@dslextreme.com> wrote: > >>>>>>>>> > >>>>>>>>> Volkan, > >>>>>>>>> > >>>>>>>>> I have updated the relevant documentation. I am done with all the > >>>> changes I wanted to make. > >>>>>>>>> > >>>>>>>>> Ralph > >>>>>>>>> > >>>>>>>>>> On Jul 6, 2020, at 2:50 PM, Ralph Goers > >>>>>>>>>> <ralph.go...@dslextreme.com> > >>>> wrote: > >>>>>>>>>> > >>>>>>>>>> I have fixed the issue with the annotations. I have added the > >>>> ability to format the message field using a pattern and I have modified > >>>> the > >>>> code so that specifying eventDelimiter=“null” will insert a null at the > >>>> end > >>>> of the message as specifying eventDelimiter=“\0” or “\u000” does not work > >>>> (It ends up being “\\0” or “\u000”. > >>>>>>>>>> > >>>>>>>>>> I have verified that with these changes the message is formatted > >>>> properly with Gelf. I am still testing though to see how the > >>>> ThreadContext > >>>> is being handled. > >>>>>>>>>> > >>>>>>>>>> Ralph > >>>>>>>>>> > >>>>>>>>>>> On Jul 5, 2020, at 11:01 PM, Ralph Goers < > >>>> ralph.go...@dslextreme.com> wrote: > >>>>>>>>>>> > >>>>>>>>>>> I finally got some time to start testing JsonTemplateLayout > >>>> against the logging in the cloud documentation. The first problem I ran > >>>> into is that additional fields don’t work. I don’t see any configurations > >>>> that have them and the Builder doesn’t have annotations on the key and > >>>> value fields so I suspect it just doesn’t work. Why didn’t you just > >>>> enhance KeyValuePair to add the type attribute? > >>>>>>>>>>> > >>>>>>>>>>> After I corrected the EventTemplateAdditionalField plugin I still > >>>> can’t get stack traces the way I want them. I thought you said you added > >>>> the ability to format the message using a pattern, but I don’t see that > >>>> in > >>>> the documentation or in MessageResolver. Was that not included? As I > >>>> said, > >>>> I require the stack trace to print in the message field in Kibana, not > >>>> just > >>>> be a key in the additional data. > >>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>>> Ralph > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>>> > >>>>> > >>>> > >>>> > >>>> -- > >>> Matt Sicker <boa...@gmail.com> > >> > >> > > > >
