1
Vote

XML Parser is too restrictive in properties section

description

I am importing the log4j file that I've attached to issue:

https://yalvlib.codeplex.com/workitem/19

and am getting an exception being shown (with the loading being ended). In one way thats OK because it means the error handling is working now.

But I don't think we should do like it like that - in this particular case - since I did not setup the file - it was actually written like this by log4net. I attach the config file from Edi so you could generate the same format if you download and execute Edi with this config file.

I propose to log a warning instead of throwing an exception for those cases that can be circumvented (like this one). The collection of warnings should be shown to the user whenever processing steps like load, save, export, load analysis session are finished with non-zero entries.

My problem at this point is that I cannot test without having to edit the code (I have to uncomment the line number line that was reported in the other issue). So, please fix to enable us to continue testing with same conditions.

file attachments

comments

gwentreb wrote Jul 3, 2013 at 7:20 AM

Actually it's not that we are too restrictive. It's just that we are trying to convert a "?" to an integer. We can admit that, if we are given a "?" we put the value to zero; wich is like an undefined value for the line number.

dirkster wrote Jul 3, 2013 at 6:08 PM

Yes we could the question mark replacement for the particular line number problem but it would not fix the issue as such - which becomes clearer if you look at the attached screenshot.

I do not know why I have these '?' characters in the properties section - I am not a log4net expert (just want to use it since its awesome) but I am 100% positive I did not put them there.

Now I've been having this dead situation and I think we should make sure other people do not experience the same problem just because we have been too restive here. I think we should always try to escape the element that is giving us trouble, for example:
  • Set a default value if a conversion fails and skip that particular value (log a warning)
  • Skip reading the properties section if it appears to be formatted wrong (log a warning)
  • Skip reading a log4net entry if it appears to be formatted wrong (log a warning)
This way the tool is strict in the import format (which should help us figure out problems to come) but is still relaxed enough to do what it can.

We should either do something like this or have a configurable option (ImportInAnyCase=true) that will run over exceptions... otherwise, I am sure, we are too restrictive which would be a shame for everyone else :-(

gwentreb wrote Jul 4, 2013 at 7:32 AM

I'm ok for setting a default value (that's pretty much what i've made for the convertion into an uint value)
But do we really want to lose any informations about our logs.

I mean, imagine that the machine producing the logs is unable to know which method / file / class generated that entry. We still want to see the entry in our dataview.

And since it's the system that try to parse the xml file and not one of our method, if it's formatted wrong we raise an exception telling where it's bad formatted (line + position) I don't think we should do anything more. If the data files are not properly produced it's not really our problem I think.

Beside that, I totally agree about logging the potential errors we might encounter. So maybe we could implement a logging system using log4net in our log4net logs analyzer :D

dirkster wrote Jul 4, 2013 at 3:44 PM

And since it's the system that try to parse the xml file and not one of our method, if it's formatted wrong
we raise an exception telling where it's bad formatted (line + position) I don't think we should do
anything more. If the data files are not properly produced it's not really our problem I think.
I don't think just raising an exception is a good idea. Exception should only be raised in exceptional cases (for example if the file has a length of zero, is not accessible, or contains only binary data).

1) What do you expect me or Francois to do if we get this exception?
1.1 Look through your code and un-comment the offending line?
1.2 Look through a log file of 5000 line (and maybe 100s of offending lines)
      and remove the offending lines?
1.3 Give up and start using the software from someone else?
2) I did nothing tricky. Just generated a standard log4net output. That does not justify an exception that terminates in blocking behavior. No format problem does unless its really bad (like 100s of miss-formatted lines that have not matchable content).

3) I was able to view this standard log4net output in the last released version and I expect nothing less.

gwentreb wrote Jul 5, 2013 at 9:47 AM

I still think we should never remove the offending lines. Just warn, once the file has been parsed which entries are missing informations and so on, but still keep them in the view with empty values where the information is missing.

I will implement something based on exception report you show us on EDI to show these informations.

So exceptions will be raised only if the parser can't parse the file (missing tag or stuff like this).

dirkster wrote Jul 8, 2013 at 7:26 PM

That sound good :) Like I said the simplest version would be to display a summary of warnings and errors and refer to a log4net file - but do not block processing unless its absolutely necessary of course.

(the idea of using log4net logging in a log4net viewer application is not so original - it was the reason why I wanted this in Edi and it helps me every day so it was a worthwhile effort for me)