search

HappyPorch / uSplit

A/B Testing plugin for Umbraco
The Unlicense
11 stars 7 forks source link

Add try-catch around JSON deserialization #17

Closed hakon-knowit closed 7 years ago

hakon-knowit commented 7 years ago

Fixes issue #15

The problem was not UTF8 after all. Instead the problem was that uSplit would always treat the experiment description as a json string.

This commit only wraps the deserialization in a try-catch clause and logs the exception.

A better fix would be to only deserialize the description when it contains the description separator. That would be a breaking change, however.

ondrejpialek commented 7 years ago

Hello @hakon-knowit, thanks a lot for this excellent contribution that fixes the issue you previosuly run into.

Two things:

  1. It seems like there is nothing to deserialize if the "separator" string is not found, so we don't even have to attempt it. Could you just add an else clause to the separator check and a return statementwith the default value if the separator is not found. I think a try/catch block should stay as well, though I might move it to the calling method so that I can add more context (experiment ID).
  2. Would you mind issuing this PR against develop? I will then merge into master and u7.5.

I am happy to do all this myself this evening, in case you do have time. Let me know what you prefer.

Thanks again! Ondrej.

ondrejpialek commented 7 years ago

Just to clarify - it will not be a breaking change because the first time this form of metadata saving was introduced it already included this separator. It would only break if someone went and deleted the separator from the description manually. :)

hakon-knowit commented 7 years ago

Hi @ondrejpialek

I can update the pull request tomorrow morning when I'm back at work, or you can do the changes yourself if you want to. I don't mind either way.

I was hesitant to introduce changes that would break other people's setup, so it's good to hear it won't be a problem.

Thanks for your quick response :)

hakon-knowit commented 7 years ago

@ondrejpialek I've changed the base branch to develop and made sure separator is present before parsing settings.

I kept the try-catch block in ParseSettings() since it is called from two locations in the code and I did not want to make too many changes.

ondrejpialek commented 7 years ago

Hi @hakon-knowit, thanks again for the great PR and for fixing this issue.

I have accepted it and merged into master (for U7.6+) and u7.5, once these get built on our CI server they should get published to NuGet, shouldn't take more than an hour I'd say.

Many thanks! Ondrej.