www.digitalmars.com         C & C++   DMDScript  

digitalmars.D - std.json API broken without notice

reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
std.json broke backward compatibility, starting with 
https://github.com/D-Programming-Language/phobos/commit/1958c95666b0241d669d2
2806e4f724fbb37caf. 
There's even no notice in the changelog of the breakage, which is quite 
obvious by inspecting the diff.

This is a serious matter because it broke production code. Was there a 
reason for the breakage? We should make sure breaking changes are 
avoided or get a lot of scrutiny if they are really necessary.

I wonder how we can improve the process to avoid such issues in the future.


Andrei
Mar 08 2014
next sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Sunday, 9 March 2014 at 03:58:31 UTC, Andrei Alexandrescu 
wrote:

 std.json broke backward compatibility, starting with 
 https://github.com/D-Programming-Language/phobos/commit/1958c95666b0241d669d282806e4f724fbb37caf.


Pull request link with discussion:
https://github.com/D-Programming-Language/phobos/pull/1421


 There's even no notice in the changelog of the breakage, which 
 is quite obvious by inspecting the diff.


Changelogs should be generated from pull requests, not Bugzilla 
issues anyway.
https://d.puremagic.com/issues/show_bug.cgi?id=12240


 This is a serious matter because it broke production code. Was 
 there a reason for the breakage? We should make sure breaking 
 changes are avoided or get a lot of scrutiny if they are really 
 necessary.


I don't think it was an intentional change, rather a mundane 
regression. Could you post the affected code?


 I wonder how we can improve the process to avoid such issues in 
 the future.


Make it easy for individuals and companies to set up CI servers 
which automatically test their projects with beta versions of D?
Mar 08 2014
next sibling parent reply Walter Bright <newshound2 digitalmars.com> writes:
On 3/8/2014 8:08 PM, Vladimir Panteleev wrote:

 On Sunday, 9 March 2014 at 03:58:31 UTC, Andrei Alexandrescu wrote:

 I wonder how we can improve the process to avoid such issues in the future.


 Make it easy for individuals and companies to set up CI servers which
 automatically test their projects with beta versions of D?


The coverage of the unittests for std.json is 92%. But most of the uncovered 
lines should be trivial to cover with unittests. For example,
JSON_TYPE.UINTEGER 
is completely untested.

A review of unittest coverage should be part of any review of new library code.

Note that checking the unittest coverage is as easy as:

     dmd std/json -unittest -main -cov
Mar 08 2014
parent reply Walter Bright <newshound2 digitalmars.com> writes:
BTW, I don't know if better unittest coverage would have detected this 
particular breakage, but in any case, we should do better with coverage.
Mar 08 2014
parent reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 3/9/14, Walter Bright <newshound2 digitalmars.com> wrote:

 BTW, I don't know if better unittest coverage would have detected this
 particular breakage, but in any case, we should do better with coverage.


Can someone finally say *what* broke?
Mar 09 2014
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/9/14, 6:49 AM, Andrej Mitrovic wrote:

 On 3/9/14, Walter Bright <newshound2 digitalmars.com> wrote:

 BTW, I don't know if better unittest coverage would have detected this
 particular breakage, but in any case, we should do better with coverage.


 Can someone finally say *what* broke?


I was wrong in my previous message. As Adam noted, it was making type 
read-only. Previously people would write:

v.type = JSON_TYPE.STRING;
v.str = "abc";

Now the second line sets the type, and the first is an error.

Also, this code is in error:

v.type = JSON_TYPE.TRUE;

and must be replaced with:

v = true;


Andrei
Mar 09 2014
next sibling parent "w0rp" <devw0rp gmail.com> writes:
On Sunday, 9 March 2014 at 17:30:32 UTC, Andrei Alexandrescu 
wrote:

 v.type = JSON_TYPE.TRUE;

 and must be replaced with:

 v = true;


 Andrei


I see why you'd want to make this kind of change. It was probably 
a mistake to let you change the tag like that, what could be done 
to fix this is this.

deprecated("Don't assign to type!")  property void type(JSON_TYPE 
new_type)
{
     type_tag = new_type;
}

Then the code would probably work like before and people using 
x.type = ... in production would start seeing a deprecation 
warning instead, which we could make into an error at a later 
date.
Mar 09 2014
prev sibling parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Sunday, 9 March 2014 at 17:30:32 UTC, Andrei Alexandrescu 
wrote:

 On 3/9/14, 6:49 AM, Andrej Mitrovic wrote:

 On 3/9/14, Walter Bright <newshound2 digitalmars.com> wrote:

 BTW, I don't know if better unittest coverage would have 
 detected this
 particular breakage, but in any case, we should do better 
 with coverage.


 Can someone finally say *what* broke?


 I was wrong in my previous message. As Adam noted, it was 
 making type read-only. Previously people would write:

 v.type = JSON_TYPE.STRING;
 v.str = "abc";

 Now the second line sets the type, and the first is an error.

 Also, this code is in error:

 v.type = JSON_TYPE.TRUE;

 and must be replaced with:

 v = true;


I think this is something that can be easily fixed without 
causing more regressions. Have you filed it as an issue yet?
Mar 09 2014
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/9/14, 10:43 AM, Vladimir Panteleev wrote:

 I think this is something that can be easily fixed without causing more
 regressions. Have you filed it as an issue yet?


https://d.puremagic.com/issues/show_bug.cgi?id=12332

Andrei
Mar 09 2014
parent reply "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Sunday, 9 March 2014 at 17:51:54 UTC, Andrei Alexandrescu 
wrote:

 On 3/9/14, 10:43 AM, Vladimir Panteleev wrote:

 I think this is something that can be easily fixed without 
 causing more
 regressions. Have you filed it as an issue yet?


 https://d.puremagic.com/issues/show_bug.cgi?id=12332


https://github.com/D-Programming-Language/phobos/pull/1988
Mar 09 2014
parent Walter Bright <newshound2 digitalmars.com> writes:
On 3/9/2014 11:18 AM, Vladimir Panteleev wrote:

 https://github.com/D-Programming-Language/phobos/pull/1988


Thanks for the quick action, Vladimir!
Mar 09 2014
prev sibling parent reply Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 3/9/14, Vladimir Panteleev <vladimir thecybershadow.net> wrote:

 I don't think it was an intentional change, rather a mundane
 regression. Could you post the affected code?


Breaking code wasn't intended.

On 3/9/14, Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> wrote:

 I wonder how we can improve the process to avoid such issues in the future.


The obvious answer is to have proper unittests. If the API broke, the
existing tests should have caught this. But the tests weren't touched
in the pull request, only new ones were added. What exactly broke
though?
Mar 09 2014
parent reply Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/9/14, 6:23 AM, Andrej Mitrovic wrote:

 On 3/9/14, Vladimir Panteleev <vladimir thecybershadow.net> wrote:

 I don't think it was an intentional change, rather a mundane
 regression. Could you post the affected code?


 Breaking code wasn't intended.

 On 3/9/14, Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> wrote:

 I wonder how we can improve the process to avoid such issues in the future.


 The obvious answer is to have proper unittests. If the API broke, the
 existing tests should have caught this. But the tests weren't touched
 in the pull request, only new ones were added. What exactly broke
 though?


Apparently direct use of the untagged union elements was not being 
unittested.

Andrei
Mar 09 2014
parent "Vladimir Panteleev" <vladimir thecybershadow.net> writes:
On Sunday, 9 March 2014 at 17:25:31 UTC, Andrei Alexandrescu 
wrote:

 Apparently direct use of the untagged union elements was not 
 being unittested.


Were they documented? How did their use end up in production code?
Mar 09 2014
prev sibling next sibling parent reply David <d dav1d.de> writes:
Am 09.03.2014 04:58, schrieb Andrei Alexandrescu:

 std.json broke backward compatibility, starting with
 https://github.com/D-Programming-Language/phobos/commit/1958c95666b0241d669d282806e4f724fbb37caf.
 There's even no notice in the changelog of the breakage, which is quite
 obvious by inspecting the diff.
 
 This is a serious matter because it broke production code. Was there a
 reason for the breakage? We should make sure breaking changes are
 avoided or get a lot of scrutiny if they are really necessary.
 
 I wonder how we can improve the process to avoid such issues in the future.
 
 
 Andrei


What code did break?
I did use std.json heavily myself before this pull request and it was
basically unusable (hence the pull to improve the api), so I tried to
expand std.json without breaking backwards compatability, the tests ran
through also my code still worked.
Mar 09 2014
next sibling parent "Adam D. Ruppe" <destructionator gmail.com> writes:
On Sunday, 9 March 2014 at 12:35:11 UTC, David wrote:

 What code did break?


It made the type field a read-only thing which broke building 
json structures.
Mar 09 2014
prev sibling parent Andrei Alexandrescu <SeeWebsiteForEmail erdani.org> writes:
On 3/9/14, 5:35 AM, David wrote:

 Am 09.03.2014 04:58, schrieb Andrei Alexandrescu:

 std.json broke backward compatibility, starting with
 https://github.com/D-Programming-Language/phobos/commit/1958c95666b0241d669d282806e4f724fbb37caf.
 There's even no notice in the changelog of the breakage, which is quite
 obvious by inspecting the diff.

 This is a serious matter because it broke production code. Was there a
 reason for the breakage? We should make sure breaking changes are
 avoided or get a lot of scrutiny if they are really necessary.

 I wonder how we can improve the process to avoid such issues in the future.


 Andrei


 What code did break?
 I did use std.json heavily myself before this pull request and it was
 basically unusable (hence the pull to improve the api), so I tried to
 expand std.json without breaking backwards compatability, the tests ran
 through also my code still worked.


As is plain from the diff, the publicly available storage now needs 
".store" in there. Making untagged store public was probably a mistake 
in the initial design, but breakage is what it is.

Andrei
Mar 09 2014
prev sibling parent reply "Jakob Ovrum" <jakobovrum gmail.com> writes:
On Sunday, 9 March 2014 at 03:58:31 UTC, Andrei Alexandrescu 
wrote:

 std.json broke backward compatibility, starting with 
 https://github.com/D-Programming-Language/phobos/commit/1958c95666b0241d669d2
2806e4f724fbb37caf. 
 There's even no notice in the changelog of the breakage, which 
 is quite obvious by inspecting the diff.

 This is a serious matter because it broke production code. Was 
 there a reason for the breakage? We should make sure breaking 
 changes are avoided or get a lot of scrutiny if they are really 
 necessary.

 I wonder how we can improve the process to avoid such issues in 
 the future.


 Andrei


While this breakage was gratuitous, I would have expected 
std.json to have this infamous warning by now:

"Warning: This module is considered out-dated and not up to 
Phobos' current standards. It will remain until we have a 
suitable replacement, but be aware that it will not remain long 
term."

It's a pretty crap module and it's a fact that we are looking for 
a replacement. Maybe this caused everyone involved to lower their 
standard for proper unit tests and general scrutiny.
Mar 13 2014
parent Andrej Mitrovic <andrej.mitrovich gmail.com> writes:
On 3/13/14, Jakob Ovrum <jakobovrum gmail.com> wrote:

 It's a pretty crap module and it's a fact that we are looking for
 a replacement.


It clearly didn't have the proper initial unittests for the API which
broke. The pull request which introduced the regression did not modify
existing unittests, it only added new ones. The pull went through a
review just like any other pull request, there's nobody going "oh this
updates std.xml, I'll just merge this regardless because std.xml
sucks".
Mar 13 2014