search

Brewtarget / brewtarget

Main brewtarget source code repository.
GNU General Public License v3.0
313 stars 135 forks source link

beerxml import seems broken #500

Closed ghost closed 3 years ago

ghost commented 3 years ago

Hallo Brewtarget developpers!

I have been trying importing my old beer recipes but it always crashes. Fixing one issue, another one shows up and I'm wondering if someone have tried this feature recently that seems totally broken. The following beer recipes always fail to import, despite they were exported with brewtarget: https://gitlab.com/morandg/GuixBeers/-/tree/master/recipes

I don't know if I'm doing something wrong. I wish I could fix and PR myself but I'm getting lost in the issues from issues nightmare, any pointer is strongly appreciated.

Best regards,

Guy

ghost commented 3 years ago

Last backtrace from original code I could get: ` Thread 1 "brewtarget" received signal SIGSEGV, Segmentation fault. 0x00005555556b0032 in Ingredient::isValid (this=0x0) at /home/random/sources/brewtarget/src/ingredient.cpp:281 281 return _valid; (gdb) bt

0 0x00005555556b0032 in Ingredient::isValid (this=0x0) at /home/random/sources/brewtarget/src/ingredient.cpp:281

1 0x00005555556c219a in BeerXML::recipeFromXml (this=0x555555e9f600, node=...) at /home/random/sources/brewtarget/src/beerxml.cpp:1326

2 0x0000555555755d2d in Database::importFromXML (this=0x555555e72e40, filename=...) at /home/random/sources/brewtarget/src/database.cpp:3051

3 0x000055555580b8e4 in MainWindow::importFiles (this=0x555556188b40) at /home/random/sources/brewtarget/src/MainWindow.cpp:1944

4 0x000055555572f18e in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (MainWindow::)()>::call(void (MainWindow::)(), MainWindow*, void*) (f=(void (MainWindow::)(MainWindow * const)) 0x55555580b7de <MainWindow::importFiles()>, o=0x555556188b40, arg=0x7fffffffd8d0)

at /usr/include/qt5/QtCore/qobjectdefs_impl.h:152

5 0x000055555572eb9c in QtPrivate::FunctionPointer<void (MainWindow::)()>::call<QtPrivate::List<>, void>(void (MainWindow::)(), MainWindow*, void**) (

f=(void (MainWindow::*)(MainWindow * const)) 0x55555580b7de <MainWindow::importFiles()>, o=0x555556188b40, arg=0x7fffffffd8d0)
at /usr/include/qt5/QtCore/qobjectdefs_impl.h:185

6 0x000055555572e692 in QtPrivate::QSlotObject<void (MainWindow::)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase, QObject*, void*, bool) (which=1,

this_=0x555556c35140, r=0x555556188b40, a=0x7fffffffd8d0, ret=0x0) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:418

7 0x00007ffff6b6cfee in ?? () from /usr/lib/libQt5Core.so.5

8 0x00007ffff77f6b22 in QAction::triggered(bool) () from /usr/lib/libQt5Widgets.so.5

9 0x00007ffff77f9148 in QAction::activate(QAction::ActionEvent) () from /usr/lib/libQt5Widgets.so.5

10 0x00007ffff797ecb2 in ?? () from /usr/lib/libQt5Widgets.so.5

11 0x00007ffff79862ea in ?? () from /usr/lib/libQt5Widgets.so.5

12 0x00007ffff7987582 in QMenu::mouseReleaseEvent(QMouseEvent*) () from /usr/lib/libQt5Widgets.so.5

13 0x00007ffff783ef1e in QWidget::event(QEvent*) () from /usr/lib/libQt5Widgets.so.5

14 0x00007ffff7989b73 in QMenu::event(QEvent*) () from /usr/lib/libQt5Widgets.so.5

15 0x00007ffff77fcd6f in QApplicationPrivate::notify_helper(QObject, QEvent) () from /usr/lib/libQt5Widgets.so.5

16 0x00007ffff7805ce7 in QApplication::notify(QObject, QEvent) () from /usr/lib/libQt5Widgets.so.5

17 0x00007ffff6b3738a in QCoreApplication::notifyInternal2(QObject, QEvent) () from /usr/lib/libQt5Core.so.5

18 0x00007ffff7804f13 in QApplicationPrivate::sendMouseEvent(QWidget, QMouseEvent, QWidget, QWidget, QWidget**, QPointer&, bool, bool) ()

from /usr/lib/libQt5Widgets.so.5

19 0x00007ffff785a52e in ?? () from /usr/lib/libQt5Widgets.so.5

20 0x00007ffff785cd8c in ?? () from /usr/lib/libQt5Widgets.so.5

21 0x00007ffff77fcd6f in QApplicationPrivate::notify_helper(QObject, QEvent) () from /usr/lib/libQt5Widgets.so.5

22 0x00007ffff7805a20 in QApplication::notify(QObject, QEvent) () from /usr/lib/libQt5Widgets.so.5

23 0x00007ffff6b3738a in QCoreApplication::notifyInternal2(QObject, QEvent) () from /usr/lib/libQt5Core.so.5

24 0x00007ffff6f5e38b in QGuiApplicationPrivate::processMouseEvent(QWindowSystemInterfacePrivate::MouseEvent*) () from /usr/lib/libQt5Gui.so.5

25 0x00007ffff6f5f8d5 in QGuiApplicationPrivate::processWindowSystemEvent(QWindowSystemInterfacePrivate::WindowSystemEvent*) () from /usr/lib/libQt5Gui.so.5

26 0x00007ffff6f3800b in QWindowSystemInterface::sendWindowSystemEvents(QFlags) () from /usr/lib/libQt5Gui.so.5

27 0x00007ffff2d172aa in ?? () from /usr/lib/qt5/plugins/platforms/../../../libQt5XcbQpa.so.5

28 0x00007ffff556122d in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0

29 0x00007ffff5562d90 in ?? () from /usr/lib/libglib-2.0.so.0

30 0x00007ffff5562dcf in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0

31 0x00007ffff6b8fdbe in QEventDispatcherGlib::processEvents(QFlags) () from /usr/lib/libQt5Core.so.5

32 0x00007ffff6b35f7b in QEventLoop::exec(QFlags) () from /usr/lib/libQt5Core.so.5

33 0x00007ffff6b3de90 in QCoreApplication::exec() () from /usr/lib/libQt5Core.so.5

34 0x000055555571fcb3 in Brewtarget::run (userDirectory=...) at /home/random/sources/brewtarget/src/brewtarget.cpp:636

35 0x0000555555680520 in main (argc=1, argv=0x7fffffffea88) at /home/random/sources/brewtarget/src/main.cpp:73`

matty0ung commented 3 years ago

This is a very interesting bug. I have a fix for it which I will post soon.

matty0ung commented 3 years ago

So, the fun bit is fixed in https://github.com/Brewtarget/brewtarget/pull/501.

@r4nd0m6uy There are still problems loading in the files you link to, but I don't yet know enough about what expected behaviour is. Specifically, once the above fix is applied, importing the file 'recipes_Guix-5.0.0.xml' blows up because the code cannot handle a MashStep that doesn't have a name. (If you manually edit the XML to insert random text into below then the import appears to go OK.) If a MashStep must always have a name then we ought to at least display a friendly error message (to allow the user to fix the file) and/or possibly create auto-generated names (to let the import complete). If a MashStep does not need a name then I think we have one database constraint too many.

ghost commented 3 years ago

Hoy @matty0ung,

Thanks for your quick reply and quick fix! Indeed, I would not have found it myself in less than one day :stuck_out_tongue: ...

Singletone ... mmm hhh ... For the last issue, I don't mind editing my XML and can live with it, although I fix is very welcome!

Thanks and keep the great work!

ghost commented 3 years ago

Didn't test myself though, I guess you did and you can close the issue if you feel confident :+1:

matty0ung commented 3 years ago

No worries. :-)

For the rest of the issue, my instinct is that, when reading files, the software should try to "make sense" as best it can of the input - so, in this case, generate some arbitrary name to replace the 'missing' one and carry on processing the rest of the file. (Conversely, for writing files, we should obviously aim to generate exemplary BeerXML.) If others agree then I would be happy to have a go at making the file reading really robust in this way, but it's probably a non-trivial change, so I think I ought to wait for @mikfire to comment/approve my current 'big' pull request (https://github.com/Brewtarget/brewtarget/pull/499) before I dive into it.

ghost commented 3 years ago

I just want to confirm that I have tested the patch and after fixing my recipe as suggested, it works! https://gitlab.com/morandg/GuixBeers/-/commit/d5f4220e7dab4c45931f4432c063fc14bbdfea03

I would like to apologize for the aggressive post, I was very frustrated that I couldn't tweak my recipe, now I'm very happy and will order my ingredients!

Thanks again @matty0ung , you are awesome! Long live to open source and nice communities!

matty0ung commented 3 years ago

You're welcome. Always glad to help a fellow brewer! :+1:

mikfire commented 3 years ago

If you ever wanted to know what I hate about BeerXML, it is this. Nobody generates BeerXML to spec, and the spec is written in a way that we cannot use validators on the input.

Technically, this bug is in the generating software. The BeerXML spec says NAME is a required attribute in a MASH_STEP. Yes, our code should be more defensive, but I think we should at least be able to assume required fields are present.

matty0ung commented 3 years ago

@mikfire I hear what you're saying. At the same time, it seems on this occasion that we should say nice things about the generating software because line two of the file being imported says 'BeerXML generated by brewtarget'. :smiley:

mattiasmaahl commented 3 years ago

can we consider this Issue Resolved? or is further development on the import necessary?

matty0ung commented 3 years ago

Yes, I think given that, with a bit of manual editing, OP was able to get brewing after the fix, then we can close this. I will raise a separate enhancement issue to make the XML input more robust.

ghost commented 3 years ago

Thank you guys! If I dare giving my opinion to reply @mikfire comment:

If you ever wanted to know what I hate about BeerXML, it is this. Nobody generates BeerXML to spec, and the spec is written in a way that we cannot use validators on the input.

Technically, this bug is in the generating software. The BeerXML spec says NAME is a required attribute in a MASH_STEP. Yes, our code should be more defensive, but I think we should at least be able to assume required fields are present.

I don't think BeerXML neither XML is bad, it's just that XML is very often missused or not well understood. It seems that no one validate its XML against the (BeerXML) schema that results in the mess it is today... Or should we send a feature request for a better BeerXML XSD?

matty0ung commented 3 years ago

One thing I've done in the past with importing XML, which could help us a bit, is to parse the file twice. The first time you validate and can let the user know if there are any heinous errors that you can't deal with. The second time, if the first time was OK, you actually import the data. That way you don't, say, get half-way through injecting a new recipe in your database before hitting an unrecoverable problem.

ghost commented 3 years ago

Checking again the specification from http://beerxml.com/

It seems that it was not done by XML experts. I don't find any DTD neither an XSD to validate against but a text document that vaguely describes the formate, now all this mess makes sense. Althought @matty0ung does all the best he can, he will probably never produce something viable. Instead of fixing weird issues in brewtarget, the problem should be fixed at the root, this means trying to collaborate with original authors and submit an XSD. Or did I miss something?!

Out of topics but some thoughts on my side ... :thinking:

mikfire commented 3 years ago

I tried fixing this problem a long time ago by creating an XSD from that doc. If I understood everything correctly, XSD can work with tags in random order or with optional tags. It cannot work with both. BeerXML has both and so XSD cannot be made to work.

My intense dislike for BeerXML isn't because it is XML. BeerXML is bad XML. I've asked a few of the other projects informally, and there is no interest in trying to work on beerXML v2.0, which would be the only way to fix the issue.

I would prefer BeerXML to die in fire, to be replaced by BeerJSON. Assuming that project actually wrote the schema files.

matty0ung commented 3 years ago

I am having a play around with the XSDs reverse-engineered from the BrewXML spec by Jon Lochner (@brewpoo), but it's good to know the limitations that you encountered @mikfire. I do have an idea about how we might nonetheless end up with something useful, but it needs some more investigation. I think it will be something fun to keep me occupied over the Christmas break. Will post results in https://github.com/Brewtarget/brewtarget/issues/504 as and when I have any conclusions.

Shall we make a separate issue (if there is not one already) for supporting https://github.com/beerjson/beerjson format?

mattiasmaahl commented 3 years ago

I would say that beerJSON deserves a separate issue/feature-request on its own.