Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Hi Adrian, Sorry for the delay. I've had a look at your patch. First of all, thanks for taking care of this. Strong validation of user config files is a great step towards user-friendliness. Thanks also for thinking of documentation and testcases! I have a few comments, some of which are open for discussion with other devs: - we may discuss about the interest of a separate/yet another parameter for validating the config file. I can understand both points of vue and, actually, I don't really care of which is chosen. That said, I think validation of the config file should be enabled by default (as strict FO validation, IIC) because this will be helpful to newbies. Power users will be clever enough to disable it if they want to. So I suggest you to change the default value to true in your patch. - the changes in src/documentation/content/xdocs/0.93/fonts.xml are a bit inexact. That's ok, you're probably still learning. But: - the examples you added have different noticeable behaviours: the first one gets the font's metrics through a beforehand generated XML file and won't embed the font in the PDF file, because it doesn't have access to the font file; the second ones computes the metrics on the fly and is able to embed the font in the PDF file. As those differences are not trivial and are explained further on the page I would leave the example as is. - if both files are specified the metrics-url won't take the precedence, at least not exactly. The metrics defined in this file will override the metrics in the font (if the user modified the file by hand after generating it), but the embed-url file will always be used to load and embed the font. - AFAIK, kerning is available for PS and PDF outputs. - but as I said, thanks for updating the documentation! - I ask for other devs' opinions, but in strict user-config validation mode I would not also log an error when an exception is subsequently thrown. Reporting the problem just once is enough IMO. - In apps.FopFactory: - AFAIU the validatesStrictly() method is not related to user configuration, as the javadoc seems to imply - setUserConfig doesn't throw SAXException or IOException, but FOPException If no other fop-dev have objections to my comments above, I'd like to ask you to post a new patch with the suggested corrections. It should be ready to be applied then. Thanks, Vincent --- Additional Comments From [EMAIL PROTECTED] 2007-02-02 03:55 --- Created an attachment (id=19497) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19497action=view) main patch file (In reply to comment #3) The patch is fairly large and seems to address more than only validation of the configuration file. Can you give a summary? The patch seems quite large because I have added a new suite of configuration unit tests for the fix (these are contained in the attached zip file). A summary of the changes is given below. I am against this exception being thrown if I do not use the font arial at all: [ERROR] FOP - Exception org.apache.avalon.framework.configuration.ConfigurationException: Failed to resolve font metric-url 'arial.xml'org.apache.avalon.framework.configuration.ConfigurationException: Failed to resolve font metric-url 'arial.xml' In order for this to be the case, the user configuration would have to be (re-)read/parsed at the runtime of the rendering operation in order that it would be able to know if a particular font is being used in the given FO file. This would be an inefficient way of doing things and would require some structural changes. I am also against combining strict validation of the FO file and the user configuration; they are two different things. I would appreciate a separate option 'validate-configuration', which checks every entry in the configuration file. I did raise this as a question on forum and I remember you responded to it. http://www.nabble.com/Bad-FOP-configurations-tf3064300.html#a8522251 Chris Bowditch suggested that it be combined with strict-validation. I can see your argument for this and this was my original idea but I can also see Chris' argument too. On balance I have decided to go with your suggestion and make it separate a separate configuration option so as to not cause any confusion. I have introduced a new configuration variable called strict-configuration. I haven't called it validate-configuration because validation should always occur, its how the resulting error is handled that is the important thing (see Andreas' comments from the thread). If strict-configuration == false then FOP will simply log the error and continue to parse the user configuration. If strict-configuration == true then FOP will immediately raise an exception and will not continue trying to configure itself. I have attached a new patch file containing these changes (this
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Hi Vincent,
First of all, many thanks for your comments/feedback.
Vincent Hennebert wrote:
Hi Adrian,
Sorry for the delay. I've had a look at your patch. First of all, thanks
for taking care of this. Strong validation of user config files is
a great step towards user-friendliness. Thanks also for thinking of
documentation and testcases!
I have a few comments, some of which are open for discussion with other
devs:
- we may discuss about the interest of a separate/yet another parameter
for validating the config file. I can understand both points of vue
and, actually, I don't really care of which is chosen. That said,
I think validation of the config file should be enabled by default (as
strict FO validation, IIC) because this will be helpful to newbies.
Power users will be clever enough to disable it if they want to. So
I suggest you to change the default value to true in your patch.
My thinking behind initially switching this option off was to not break
any existing configurations, but you are right its probably best to help
the newbies and not worry too much about initially upsetting some of the
power users. I take your point and will make this change.
- the changes in src/documentation/content/xdocs/0.93/fonts.xml are
a bit inexact. That's ok, you're probably still learning. But:
- the examples you added have different noticeable behaviours: the
first one gets the font's metrics through a beforehand generated XML
file and won't embed the font in the PDF file, because it doesn't
have access to the font file; the second ones computes the metrics
on the fly and is able to embed the font in the PDF file. As those
differences are not trivial and are explained further on the page
I would leave the example as is.
I will revert this change.
- if both files are specified the metrics-url won't take the
precedence, at least not exactly. The metrics defined in this file
will override the metrics in the font (if the user modified the file
by hand after generating it), but the embed-url file will always be
used to load and embed the font.
I'm not sure this is the case (but maybe it should be..). All the
config fonts are loaded on demand by the LazyFont encapsulation. If you
look at org.apache.fop.fonts.LazyFont.load(), the metrics url does take
precedence over the embed url (see conditional tests on metricsFileName
and fontEmbedPath).
private void load(boolean fail) {
if (!isMetricsLoaded) {
try {
if (metricsFileName != null) {
/[EMAIL PROTECTED] Possible thread problem here */
FontReader reader = null;
if (resolver != null) {
Source source = resolver.resolve(metricsFileName);
if (source == null) {
String err = Cannot load font: failed to
create Source from metrics file
+ metricsFileName;
if (fail) {
throw new RuntimeException(err);
} else {
log.error(err);
}
return;
}
InputStream in = null;
if (source instanceof StreamSource) {
in = ((StreamSource) source).getInputStream();
}
if (in == null source.getSystemId() != null) {
in = new
java.net.URL(source.getSystemId()).openStream();
}
if (in == null) {
String err = Cannot load font: failed to
create InputStream from
+ Source for metrics file +
metricsFileName;
if (fail) {
throw new RuntimeException(err);
} else {
log.error(err);
}
return;
}
InputSource src = new InputSource(in);
src.setSystemId(source.getSystemId());
reader = new FontReader(src);
} else {
reader
= new FontReader(new InputSource(new
URL(metricsFileName).openStream()));
}
reader.setKerningEnabled(useKerning);
reader.setFontEmbedPath(fontEmbedPath);
reader.setResolver(resolver);
realFont = reader.getFont();
} else {
if (fontEmbedPath == null) {
throw new RuntimeException(Cannot load font.
No font URIs available.);
}
Re: DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
Hi Adrian, Adrian Cumiskey a écrit : snip/ - we may discuss about the interest of a separate/yet another parameter for validating the config file. I can understand both points of vue and, actually, I don't really care of which is chosen. That said, I think validation of the config file should be enabled by default (as strict FO validation, IIC) because this will be helpful to newbies. Power users will be clever enough to disable it if they want to. So I suggest you to change the default value to true in your patch. My thinking behind initially switching this option off was to not break any existing configurations, but you are right its probably best to help the newbies and not worry too much about initially upsetting some of the power users. I take your point and will make this change. Good point, in the future we might expect users complaining that their config files no longer work with FOP's latest release. Still, I think this is preferable for newcomers. Let's just get prepared, after the next release, to answering plenty of times the same question and creating a new FAQ entry about strict user config validation ;-) - the changes in src/documentation/content/xdocs/0.93/fonts.xml are a bit inexact. That's ok, you're probably still learning. But: - the examples you added have different noticeable behaviours: the first one gets the font's metrics through a beforehand generated XML file and won't embed the font in the PDF file, because it doesn't have access to the font file; the second ones computes the metrics on the fly and is able to embed the font in the PDF file. As those differences are not trivial and are explained further on the page I would leave the example as is. I will revert this change. - if both files are specified the metrics-url won't take the precedence, at least not exactly. The metrics defined in this file will override the metrics in the font (if the user modified the file by hand after generating it), but the embed-url file will always be used to load and embed the font. I'm not sure this is the case (but maybe it should be..). All the config fonts are loaded on demand by the LazyFont encapsulation. If you look at org.apache.fop.fonts.LazyFont.load(), the metrics url does take precedence over the embed url (see conditional tests on metricsFileName and fontEmbedPath). Yes, but if you then look at PDFFactory.makeFontFile you will see that the font file is read there for embedding in the resulting PDF file. Your initial wording is essentially right, just a bit misleading: it's true that the data in the metrics file will be given the preference for any font-related computation (glyph placement, kerning, etc.), but finally that's the original font file which is put into the PDF. - AFAIK, kerning is available for PS and PDF outputs. - but as I said, thanks for updating the documentation! - I ask for other devs' opinions, but in strict user-config validation mode I would not also log an error when an exception is subsequently thrown. Reporting the problem just once is enough IMO. I will change this. - In apps.FopFactory: - AFAIU the validatesStrictly() method is not related to user configuration, as the javadoc seems to imply This comment was added to the validatesStrictly method when Simon Pepping commented that these configuration options should be separate and I neglected to remove it. I will now remove this additional method comment. - setUserConfig doesn't throw SAXException or IOException, but FOPException I am at pains to modify public APIs so as to remain backwards compatible and not break anything... There are three public setUserConfig methods, two of which throws SAXException, IOException so I tried to leave these as is - as a FopException is a subclass of SAXException anyways :). The other method needed to be changed though to report an exception (FopException) - instead of just logging it. Ah yes, I forgot that public API issue. Well, that's a bit problematic, then, because backward compatibility is broken anyway (a method which previously didn't throw any exception will now throw one). What's frustrating is that we know the FOPException will only be thrown in strict validation mode, but we must still declare it. So even if the user disabled strict validation, they would still have to catch the exception. H. Personally I would declare a FOPException; if we are to break backward compatibility, we may as well be accurate. But I could understand that other devs don't agree with that. In which case there isn't really a clean solution: embed the FOPException into a RuntimeException, or log a fatal message. What do others think? Vincent
DO NOT REPLY [Bug 40529] - [PATCH] Possible bug in PSTextRenderer
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=40529. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=40529 [EMAIL PROTECTED] changed: What|Removed |Added Summary|Possible bug in |[PATCH] Possible bug in |PSTextRenderer |PSTextRenderer -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19498|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-07 08:01 --- Created an attachment (id=19539) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19539action=view) main patch file -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 [EMAIL PROTECTED] changed: What|Removed |Added Attachment #19539|0 |1 is obsolete|| --- Additional Comments From [EMAIL PROTECTED] 2007-02-07 08:11 --- Created an attachment (id=19540) -- (http://issues.apache.org/bugzilla/attachment.cgi?id=19540action=view) main patch file I have updated the main patch file following feedback from Vincent. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
DO NOT REPLY [Bug 41514] - [PATCH] Strict url validation of user config
DO NOT REPLY TO THIS EMAIL, BUT PLEASE POST YOUR BUG· RELATED COMMENTS THROUGH THE WEB INTERFACE AVAILABLE AT http://issues.apache.org/bugzilla/show_bug.cgi?id=41514. ANY REPLY MADE TO THIS MESSAGE WILL NOT BE COLLECTED AND· INSERTED IN THE BUG DATABASE. http://issues.apache.org/bugzilla/show_bug.cgi?id=41514 --- Additional Comments From [EMAIL PROTECTED] 2007-02-07 11:40 --- I had a short look at the patch, and I have the following comments: Do not change the 0.93 documentation but the trunk documentation: src/documentation/content/xdocs/trunk/configuration.xml, etc. TargetResolution is a property of FOUserAgent because it may easily vary with each run. It is not a good idea if you move properties around. They are in FopFactory or in FOUserAgent for a reason. See http://xmlgraphics.apache.org/fop/trunk/embedding.html. Although I did not (and will not) have enough time to look at the patch in detail, I have the feeling that it addresses more than user configuration validation. It does some refactoring. That requires careful inspection. -- Configure bugmail: http://issues.apache.org/bugzilla/userprefs.cgi?tab=email --- You are receiving this mail because: --- You are the assignee for the bug, or are watching the assignee.
