WIP: Improved Profile file parsing and error handling #493
Conversation
I modified the Profile constructor to better identify invalid or malformed Profile files and throw more informative exceptions. I will also be pushing a modification to openshot-qt to better handle these exceptions. I also modified the InterpolateBetween function in KeyFrame.cpp to remove a compilation warning.
I made my changes to InterpolateBetween in KeyFrame.cpp less intrusive.
Codecov Report
@@ Coverage Diff @@
## develop #493 +/- ##
===========================================
- Coverage 48.24% 48.22% -0.02%
===========================================
Files 128 128
Lines 9962 9965 +3
===========================================
Hits 4806 4806
- Misses 5156 5159 +3
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Mostly just a few minor, take-it-or-leave-it suggestions, but the "don't throw on unrecognized data" thing I'm pretty firm on. (In observance of Postel's Law.)
| assert(target <= right.co.X); | ||
| assert(target <= right.co.X); |
There was a problem hiding this comment.
Drop this change, please (addition of extraneous whitespace)
| QString line = in.readLine(); | ||
|
|
||
| // Trim any extra whitespace from the line | ||
| line = line.trimmed(); |
There was a problem hiding this comment.
Might be simpler to do this all in one go, like with the splitted parts a few lines down.
| QString line = in.readLine(); | |
| // Trim any extra whitespace from the line | |
| line = line.trimmed(); | |
| QString line = in.readLine().trimmed(); |
| else { | ||
| // Any other setting makes the file invalid | ||
| std::ostringstream oss; | ||
| oss << "Unrecognized setting \"" << setting << "\" encountered"; | ||
| throw std::runtime_error(oss.str()); |
There was a problem hiding this comment.
Honestly, I'm pretty wary of this. Logging an unrecognized setting (using ZmqLogger::Instance()->AppendDebugMethod()) would be OK, but not bailing on the entire file.
If an unrecognized parameter is an error, then whenever we next need to extend the format with new parameters, we'll instantly have a problem with older libopenshot versions breaking if they're fed any of the new files.
For backwardsforwards-compatibility, far better if the parser simply assumes that anything it doesn't know how to process is valid data that just isn't meant for its consumption, and continue processing the rest of the file.
| if (line.contains(QChar('=')) == false) { | ||
| throw std::runtime_error("Invalid line encountered"); | ||
| } | ||
|
|
||
| // Split current line | ||
| QStringList parts = line.split( "=" ); | ||
| std::string setting = parts[0].toStdString(); | ||
| std::string value = parts[1].toStdString(); | ||
| std::string setting = parts[0].trimmed().toStdString(); | ||
| std::string value = parts[1].trimmed().toStdString(); |
There was a problem hiding this comment.
Despite my previous comments about ignoring unrecognized settings, I'm fine with requiring that each line (except comments) contain an = character, since that can reasonably be considered part of the basic file format.
In fact, another way to go, rather than checking for .contains('='), would be to just always .split("=") the line, and then check that the resulting QStringList contains exactly two parts — no more, no less, as either would be an error.
Because a file accidentally left like this:
width=1280height=720
progressive=1
frame_rate_num=25
frame_rate_den=1
...should probably cause some sort of alarm to be raised.
(Unless the line is a description= line, maybe? Might be nice to special-case those so they're allowed to have arguments containing = signs. Hmm. I don't know, have to think on that.)
|
Thanks again, Frank. I appreciate that you not only give suggestions but
that you take the time to explain your thinking. I find it very
enlightening.
1) Addition of extraneous whitespace:
Gotcha. Not sure how that happened, but I'll revert it.
2) Consolidating string trimming:
Yup. Pretty obvious now that you point it out. Thank you.
3) Handling unrecognized settings:
Again, good point. I was approaching this from a different perspective.
Your explanation makes perfect sense. Again, thank you.
4) Handling '=' sign detection:
I like your approach of always splitting the line and I'll implement that.
We can tackle allowing '=' signs in descriptions for another time, I think.
5) Comments:
What are your thoughts about adding the ability to include comments in
profiles? It seemed like a good idea at the time, but it now feels like
overkill to me. If you see a benefit to allowing comments, I think I'll
improve my implementation to allow trailing comments to be stripped off of
otherwise valid lines.
Thanks again, Frank. I'll make these changes today.
…On Sat, Apr 4, 2020 at 4:10 PM Frank Dana ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Mostly just a few minor, take-it-or-leave-it suggestions, but the "don't
throw on unrecognized data" thing I'm pretty firm on. (In observance of Postel's
Law <https://en.wikipedia.org/wiki/Robustness_principle>.)
------------------------------
In src/KeyFrame.cpp
<#493 (comment)>:
> - assert(target <= right.co.X);
+ assert(target <= right.co.X);
Drop this change, please (addition of extraneous whitespace)
------------------------------
In src/Profiles.cpp
<#493 (comment)>:
> + QString line = in.readLine();
+
+ // Trim any extra whitespace from the line
+ line = line.trimmed();
Might be simpler to do this all in one go, like with the splitted parts a
few lines down.
⬇️ Suggested change
- QString line = in.readLine();
-
- // Trim any extra whitespace from the line
- line = line.trimmed();
+ QString line = in.readLine().trimmed();
------------------------------
In src/Profiles.cpp
<#493 (comment)>:
> + else {
+ // Any other setting makes the file invalid
+ std::ostringstream oss;
+ oss << "Unrecognized setting \"" << setting << "\" encountered";
+ throw std::runtime_error(oss.str());
Honestly, I'm pretty wary of this. *Logging* an unrecognized setting
(using ZmqLogger::Instance()->AppendDebugMethod()) would be OK, but not
bailing on the entire file.
If an unrecognized parameter is an error, then whenever we next need to
extend the format with new parameters, we'll instantly have a problem with
older libopenshot versions breaking if they're fed any of the new files.
For backwards-compatibility, far better if the parser simply assumes that
anything it doesn't know how to process is valid data that simply isn't
meant for its consumption, and continue processing the rest of the file.
------------------------------
In src/Profiles.cpp
<#493 (comment)>:
> + if (line.contains(QChar('=')) == false) {
+ throw std::runtime_error("Invalid line encountered");
+ }
// Split current line
QStringList parts = line.split( "=" );
- std::string setting = parts[0].toStdString();
- std::string value = parts[1].toStdString();
+ std::string setting = parts[0].trimmed().toStdString();
+ std::string value = parts[1].trimmed().toStdString();
Despite my previous comments about ignoring unrecognized settings, I'm
fine with requiring that each line (except comments) contain an =
character, since that can reasonably be considered part of the basic file
format.
In fact, another way to go, rather than checking for .contains(=), would
be to just always .split("=")` the line, and then check that the
resulting QStringList contains *exactly* two parts — no more, no less, as
either would be an error.
Because a file accidentally left like this:
width=1280height=720
progressive=1
frame_rate_num=25
frame_rate_den=1
...should probably cause some sort of alarm to be raised.
(Unless the line is a description= line, maybe? Might be nice to
special-case those so they're allowed to have arguments containing =
signs. Hmm. I don't know, have to think on that.)
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#493 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ANBR2HAXXAIK4M6DOKKFQOTRK6V7DANCNFSM4L4NM3MA>
.
--
- Brad Kartchner
|
|
@kartchnb Would it be possible for you to wrap up these changes so I can get this merged? Thx! |
|
@kartchnb Just pinging you again on this PR. I would love to merge this if we can make those final adjustments. 👍 |
jonoomph
changed the title
|
Merge conflicts have been detected on this PR, please resolve. |
Modified the Profile constructor to better identify invalid or malformed Profile files and throw more informative exceptions when issues are encountered. I will also be modifying openshot-qt to better handle these exceptions rather than failing silently.
This resolves an issue I had when a malformed Profile file caused a segmentation fault.
While I was in the code, I modified the InterpolateBetween function in KeyFrame.cpp to eliminate a compiler warning.