Re: svn commit: r1770547 - in /velocity/tools/trunk/velocity-tools-generic/src/main/java/org/apache/velocity/tools: ConversionUtils.java generic/DateTool.java
The DateTool should now be fixed: - the stupid bug about date formats ignoring time zone has been fixed - the new standard formats are iso / iso_tz for ISO 8601, and intl / intl_tz for the human-readable versions which use time zone IDs for formatting. See commit http://svn.apache.org/viewvc?view=revision&revision=1771916 All of this would have been simpler to code using java8 java.time or joda-time libs, but each version has enough reenginering trouble of its own. Plus, SimpleDateFormat not being thread-safe, a new format is instantiated for each parsing or rendering. All of this leaves a bit of an aftertaste of sub-optimality, I totally agree with the fact that datetimes should be formatted into users timezone as often as possible. But we still have to rely on the webapp author to convey users timezones... Claude On 26/11/2016 02:08, Michael Osipov wrote: Am 2016-11-25 um 01:08 schrieb Claude Brisson: By Olson TZ ID, I guess you refer to "zzz" (aka CET), and not "" (aka Central European Time), don't you? No, never ever use this braindead RFC three-letter timezones. They are not standardized, not unambigious. Invented in the dark ages of the Internet. See here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones They are fully supported by java.util.TimeZone. Unfortunately, SimpleDateFormat does not support them as format option. I highly recommend to drop those bits in r1771188. Ok, which means I have to replace zzz by TimeZone.getId(), correct? Yes. - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: svn commit: r1770547 - in /velocity/tools/trunk/velocity-tools-generic/src/main/java/org/apache/velocity/tools: ConversionUtils.java generic/DateTool.java
Salut Claude, Am 2016-11-29 um 17:22 schrieb Claude Brisson: The DateTool should now be fixed: - the stupid bug about date formats ignoring time zone has been fixed - the new standard formats are iso / iso_tz for ISO 8601, and intl / intl_tz for the human-readable versions which use time zone IDs for formatting. See commit http://svn.apache.org/viewvc?view=revision&revision=1771916 All of this would have been simpler to code using java8 java.time or joda-time libs, but each version has enough reenginering trouble of its own. Plus, SimpleDateFormat not being thread-safe, a new format is instantiated for each parsing or rendering. All of this leaves a bit of an aftertaste of sub-optimality, I totally agree with the fact that datetimes should be formatted into users timezone as often as possible. But we still have to rely on the webapp author to convey users timezones... just checked the commit. The new code looks really great now, clean and consistent. However, the comments have a few typos: ISO-8601, ISO 88601. Replace it with "ISO 8601" and you are done. Thanks for making Velocity even better... Merci, Michael On 26/11/2016 02:08, Michael Osipov wrote: Am 2016-11-25 um 01:08 schrieb Claude Brisson: By Olson TZ ID, I guess you refer to "zzz" (aka CET), and not "" (aka Central European Time), don't you? No, never ever use this braindead RFC three-letter timezones. They are not standardized, not unambigious. Invented in the dark ages of the Internet. See here: https://en.wikipedia.org/wiki/List_of_tz_database_time_zones They are fully supported by java.util.TimeZone. Unfortunately, SimpleDateFormat does not support them as format option. I highly recommend to drop those bits in r1771188. Ok, which means I have to replace zzz by TimeZone.getId(), correct? Yes. - To unsubscribe, e-mail: dev-unsubscr...@velocity.apache.org For additional commands, e-mail: dev-h...@velocity.apache.org
Re: 2.0 question
We use this as a QA tool to detect references that may not have been properly escaped or otherwise marked as safe. It does not run in a production environment, but does run in Selenium environment which is also under load. Given the volume of requests we always try to avoid allocating unnecessary objects. Unfortunately the Context is the only place where we can get location information for violation messages (template stack, macro stack, etc.) IMO it would be much simple to just pass the Context to all event handlers as one more standard parameter. People who don't Context parameter will not use it. It would solve the threading issue and avoid having "Handler instanceof ContextAware" checks at the same time. It seems like there is a some unnecessary work being done initializing event cartridges, etc. and all of that just for the purpose of setting RuntimeServices and Context references on them. As far as creation of executors: I understand that it was convenient to create the method "iterateOverEventHandlers" and apply it for event handlers with different parameters, but the cost of that is creation of the new executor instance for every invocation. It would be more efficient to just have a utility that returns a combined list of handlers (if needed) instead of creating one iterator for each list (application and context). Then the callback invocation code could just walk the list and call the handlers. We have run into some other inefficient places. For example ASTStringLiteral is buffering the entire content in the StringWriter. It does not work very well for large templates. That code creates a writer which buffers up the whole thing, then does a toString() on it creating another copy. Then in some cases calls substring() that creates yet another copy. I can dig up a few other things we fixed in our version if you guys are interested. Alex On Mon, Nov 28, 2016 at 11:48 PM, Claude Brisson wrote: > Hi Alex. > > Thanks for your feedback. > > On 28/11/2016 20:52, Alex Fedotov wrote: > > Hello All, > > ContextAware interface in Velocity is not really workable in a > multi-threaded environment. > The handler is a singleton and setting the Context on the singleton from > multiple threads is not going to work well, short of using some form of > thread local. > > > You are right about that. The ContextAware javadoc says: > > Important Note: Only local event handlers attached to the context (as > opposed to global event handlers initialized in the velocity.properties > file) should implement ContextAware. Since global event handlers are > singletons individual requests will not be able to count on the correct > context being loaded before a request. > > I agree that the site documentation could be clearer about it. > > Also the event handlers (such as insertion handler) are creating new > instance of executor for each inserted value which is inefficient. > > I'm not sure that using a reference insertion event handler in the first > place can be made efficient, and not sure either that the > allocation/deallocation accounts for a lot of performance loss since modern > JVMs do a really good job about allocation of small Java objects. > > The reference insertion event handler was initially meant to be a > debugging and checking tool rather than a production tool. This being said, > I known there can be some legitimate use cases to do it in production, but > can I ask what is your specific use case, out of simple curiosity? > > > Are you guys fixing any of this for 2.0? > > > It is not planned to fix the issue for 2.0, but you can easily implement a > custom implementation that will use a thread local member. > > The event management may be reviewed in a future version, but if you think > this particular point should specifically be addressed, I strongly advise > you to open an issue about it. > > > Claude > >
