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 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
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 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 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
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 next sibling parent 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
prev sibling next sibling parent 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
prev sibling next sibling 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 "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 next sibling parent "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
prev sibling next sibling parent "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
prev sibling next sibling parent "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
prev sibling 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