Fix return value for Manager::parseOpdsDom #1100
Conversation
There was a problem hiding this comment.
I am not sure what kiwix::Manager::parseOpdsDom() is supposed to return. I am also not sure why that method is declared protected. I don't know why the m_hasSearchResult, m_totalBooks, m_startIndex, and m_itemsPerPage data members of kiwix::Manager are needed and why they are public.
Therefore I think that we should address all of the above comprehensively.
There was a problem hiding this comment.
In itself, this function cannot really fails.
The doc is already parsed and all methods call on nodes return null node or empty string. (Never throw exception).
And we parse the string with strtoull which will always return a value (and set errno in case of error (we should check).
So either we have to deeply modify this function (and Book::updateFromOpds) to correctly[1] handle missing node/attribute or the bool return is useless and we should change it to void.
[1] The next question is "What is correctly`. The current version is tolerant to missing data and setup book with empty attribute in this case. Should we stop parsing of the whole stream if one attribute is missing ? Should we continue but tell (with a bool) that something (without telling what) goes wrong ?
| @@ -143,9 +143,8 @@ bool Manager::parseOpdsDom(const pugi::xml_document& doc, const std::string& url | |||
| m_totalBooks = strtoull(libraryNode.child("totalResults").child_value(), 0, 0); | |||
| m_startIndex = strtoull(libraryNode.child("startIndex").child_value(), 0, 0); | |||
| m_itemsPerPage = strtoull(libraryNode.child("itemsPerPage").child_value(), 0, 0); | |||
| m_hasSearchResult = true; | |||
There was a problem hiding this comment.
m_hasSearchResul is a member of Manager and is public, so part of the API.
Even if, indeed, it is not used in any of our project, we cannot simply not set it.
There was a problem hiding this comment.
Ok, makes sense, I have reverted that part.
| } catch(...) { | ||
| m_hasSearchResult = false; | ||
| return false; |
There was a problem hiding this comment.
The totalResults/startIndex/itemsPerPage nodes are present when parsing a OPDS which is a search result. If parsing a root opds stream, they are not present.
So we must not stop the parsing of the opds stream if they are not present.
It should probably be declared private and return
It is not clear even to me :) I suppose it was used somehow at a moment but I cannot found where/when. |
There was a problem hiding this comment.
This PR doesn't fix anything. The return value of kiwix::Manager::parseOpdsDom() is not used in any of kiwix projects. In theory it could be used by some code that inherits from kiwix::Manager but even then the fix should start from first defining the semantics of the return value of kiwix::Manager::parseOpdsDom(). It looks that a proper solution would be to change the return type to void, but that would mean a change in the API. A roughly equivalent solution is to state that the return value of kiwix::Manager::parseOpdsDom() doesn't matter.
|
@veloman-yunkan How should we know if parsing went well or not? |
A higher level function |
Fixes: #1099