Make XML import more robust

[matty0ung]

Following https://github.com/Brewtarget/brewtarget/issues/500, I'd like to make the XML import more robust:

• Current error-reporting when something goes wrong isn't as user-friendly as it might be - eg software exits with an error message to stdout...
• Some things that are, strictly, errors (eg missing mash step name) in the creation of the file are actually things that we could and ideally should recover from. We perhaps can't auto-magically parse every erroneous BeerXML file, but we should IMHO target (a) handling common errors and (b) as far as possible importing any file that was exported from a current or previous released version of Brewtarget

[mattiasmaahl]

I agree, we should be able to quite easily implement this, maybe just as a start report on the bigger categories, i.e. Mash-step, Fermentables, Equipment and such. or do you feel a more detailed error message is to be prefered? My thinking here is to utilize the logging module more, maybe rewrite part of is so that it is always writing a Debug log file, but keeping it limited to the last 100 rows for example. this could also be used for issue reporting as well if need be?

[matty0ung]

Yes, probably a pop-up window can give summary info about whether the import succeeded or failed - perhaps with first catastrophic error in the latter case - and then refer the user to the log file for blow-by-blow details.

[mattiasmaahl]

I can tinker with the logging module and see if I can figure out a good strategy for the log file rotation and the dry-run-import. That is if you're not in the process already?

[matty0ung]

Good idea. I think having log file rotation is a good think independently of XML logging. I tend to add lots of debugging logging when I'm working on a feature (and then delete some of it if it's insanely verbose), but it's relatively easy as a developer to create a large log file. :smiley:

I am starting to have a look at the XML code. Although @mikfire already did some good work to pull XML parsing out of the Database class, I think we can go further in decoupling. I am also wondering if the import process cannot validate against BeerXML schemas (available at https://github.com/brewpoo/BeerXML-Standard/tree/master/schema) using QXmlSchemaValidator. This would maybe save us reinventing some wheels to answer "is this valid BeerXML".

[mattiasmaahl]

Sound god to me, will start working on the Log file rotation, will revert when I have something.

Regarding XML Import, I agree @mikfire did a good job on the XML parsing, and if the QXmlSchemaValidator works that would be a very good time-saver, but I have to read up on that library as I have never used it before. Will revert when I have done some preliminary work.

[matty0ung]

OK, cool, if you look at log files, shall I look at XML schemas?

Also, if you get time to investigate while you're looking at logging code, it would be neat to be able to use iostreams output. In theory Qt lets you write things such as qDebug() << "Brush:" << myQBrush << "Other value:" << i; but when I tried this in Brewtarget, the output didn't go to brewtarget_log.txt

[mattiasmaahl]

ok, That sound good, You look at XML schemas and I will look at the logs.

The way Logging is set up now is, AIUI, either you use stream OR File, which one would think could be all the same, but not as it is set up now... (I may have had a hand in that. :) ) What I'm thinking is I'll rework it to use a stream anyway and then use a file-limit, so to speak, and then prune older files as logs grow. Maybe something like MAX_FILE_SIZE = 100Kb, named brewtargetlog_.log, and MAX_FILES=5 that would yield like 5 log files each 100kb in size and as the logs keep coming, then the 6th or more file would be deleted. I may be able to work in both ways, stream and direct file, though, so you can use it either way. I'll look into it and see what I come up with.

[mattiasmaahl]

I have made som good progress on the logging library. After some thinking an pondering a, more or less, rewrite of thet library was the best vhoice moving forward. This version vill retain the old Brewtarget::logX methods, although those are just wrappers for the newer qDebug, and friends. I'm also implementing the log file size restriction and file count in this.

Will have somthing up for review next week.

[mattiasmaahl]

Created a PR for the log rotation and adding the qDebug() << funtionality.

Although I did wrestle a bit with the notion that the log-levels now defined in Log_LogType maybe should be removed and replaced with the Qt levels instead?

As these enums is already in use in the config/settings I did opt to keep them for now, but thought i could ask your opinion on the matter.

[matty0ung]

It's a good question. I was wondering it myself as I looked at the code. It's possible we might want different levels in future but OTOH the Qt levels look sensible and are pretty much the same as our own. My instinct would be to lean towards using the Qt ones (just on the general principle of not have custom versions of things if there's no benefit over using the 'as is' ones from the framework/library), but @mikfire has much more experience with the code base and may know other reasons for having/retaining our custom levels.

[mattiasmaahl]

IMHO the logging levels are what they are, converting to the Qt levels now may save us work in the future. I can have a look at it tomorrow and see if an easy conversion without changing the "interface" so to speak.

[matty0ung]

For those who are following this, I have some progress to report. I am now able (with a couple of tricks) validate a BeerXML file against an XSD. But I have a question, most probably for @mikfire .

I can see in @r4nd0m6uy's recipe files that we use some extensions to BeerXML when we export recipes. Eg IS_MASHED in a FERMENTABLE record is not (AFAICT) one of the specified fields in the BeerXML 1.0 standard. Do we have a list of these extension tags? Or should I just reverse-engineer it? (I get that it's valid to add whatever tags you like to a BeerXML file, so when we validate one, we are just supposed to ignore any extra tags that we don't understand. It's just that if they are tags that Brewtarget actually uses, it would be nice to explicitly mention them in our validation schema so they don't generate warnings.)

[matty0ung]

OK, I've finally got a patch for this. Would greatly appreciate feedback and testing. (I only had a limited supply of BeerXML files to play with!)

[xzfantom]

I'll try to find time to play with it. Great work done! Now I'm building additional libs for win but don't have much time, so it can take a while.