View Issue Details

IDProjectCategoryView StatusLast Update
0036803FPCPatchpublic2020-05-02 17:50
ReporterDenis Grinyuk Assigned ToMichael Van Canneyt  
PrioritynormalSeverityminorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.0.4 
Summary0036803: TJSONStreamer ignores properties declaration order
DescriptionTJSON(De)Streamer uses TPropInfoList which returns properties in alphabetical order.
TPropInfoList calls GetPropList in constructor without Sorted parameter (set to True by default).
Additional InformationSee https://forum.lazarus.freepascal.org/index.php?topic=36323.0
TagsNo tags attached.
Fixed in Revision44776
FPCOldBugId
FPCTarget-
Attached Files

Activities

Denis Grinyuk

2020-03-19 10:31

reporter  

0001-Fix-for-TJSONStreamer-ignoring-fields-declaration-order.patch (2,529 bytes)   
From 1adc4b304ec8922f136929d0c0f7524a84523ffe Mon Sep 17 00:00:00 2001
From: Denis Grinyuk <denis.grinyuk@gmail.com>
Date: Thu, 19 Mar 2020 12:16:41 +0300
Subject: [PATCH] Fix for TJSONStreamer ignoring fields declaration order

TJSON(De)Streamer uses TPropInfoList which returns properties in alphabetical order.
TPropInfoList calls GetPropList in constructor without Sorted parameter (set to True by default).
---
 packages/fcl-base/src/rttiutils.pp  | 8 ++++----
 packages/fcl-json/src/fpjsonrtti.pp | 4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/packages/fcl-base/src/rttiutils.pp b/packages/fcl-base/src/rttiutils.pp
index 21519c4440..d4a6fcb464 100644
--- a/packages/fcl-base/src/rttiutils.pp
+++ b/packages/fcl-base/src/rttiutils.pp
@@ -47,7 +47,7 @@ type
     FSize: Integer;
     function Get(Index: Integer): PPropInfo;
   public
-    constructor Create(AObject: TObject; Filter: TTypeKinds);
+    constructor Create(AObject: TObject; Filter: TTypeKinds; Sorted: Boolean = True);
     destructor Destroy; override;
     function Contains(P: PPropInfo): Boolean;
     function Find(const AName: string): PPropInfo;
@@ -157,14 +157,14 @@ end;
 
 { TPropInfoList }
 
-constructor TPropInfoList.Create(AObject: TObject; Filter: TTypeKinds);
+constructor TPropInfoList.Create(AObject: TObject; Filter: TTypeKinds; Sorted: Boolean);
 begin
   if AObject <> nil then
     begin
-    FCount := GetPropList(AObject.ClassInfo, Filter, nil);
+    FCount := GetPropList(AObject.ClassInfo, Filter, nil, Sorted);
     FSize := FCount * SizeOf(Pointer);
     GetMem(FList, FSize);
-    GetPropList(AObject.ClassInfo, Filter, FList);
+    GetPropList(AObject.ClassInfo, Filter, FList, Sorted);
     end
   else
     begin
diff --git a/packages/fcl-json/src/fpjsonrtti.pp b/packages/fcl-json/src/fpjsonrtti.pp
index b76a0f2f9d..023faec942 100644
--- a/packages/fcl-json/src/fpjsonrtti.pp
+++ b/packages/fcl-json/src/fpjsonrtti.pp
@@ -550,7 +550,7 @@ begin
     JSONToCollection(JSON, AObject as TCollection)
   else
     begin
-    Pil:=TPropInfoList.Create(AObject,tkProperties);
+    Pil:=TPropInfoList.Create(AObject,tkProperties,False);
     try
       For I:=0 to PIL.Count-1 do
         begin
@@ -780,7 +780,7 @@ begin
       Result.Add('Objects', StreamTList(TList(AObject)))
     else
       begin
-      PIL:=TPropInfoList.Create(AObject,tkProperties);
+      PIL:=TPropInfoList.Create(AObject,tkProperties,False);
       try
         For I:=0 to PIL.Count-1 do
           begin
-- 
2.19.1.windows.1

Michael Van Canneyt

2020-03-19 10:46

administrator   ~0121646

In difference with XML, JSON properties do not have an order. It is a hash.
The order depends on the hash mechanism used to store properties.
Any code relying on a specific order is by definition wrong.

So this patch will not be applied.

Denis Grinyuk

2020-04-01 12:17

reporter   ~0121826

You are right. JSON does not require strict fields order. But! JSON keeps human readability as its priority point. And fields order is a part of this readability.

Look at my real example. I return JSON with two fields "error" and "data". Data field can hold different structures and they can be rather long. I want "error" to be the first field. So one can see if it is zero (no errors) without scrolling to the end. Now I have no way to do that - "data" always goes before "error". Alphabetically! (((

I think this patch should be included to provide some control over human readability. For TJSONStreamer at least.

P.S. We can also add some "Sorted" property to TJSONStreamer for complete backward compatibility. If it does matter.

Michael Van Canneyt

2020-04-01 12:57

administrator   ~0121830

The basic TJSON object used in the streamer does not guarantee sorted properties.
And I think it's simply plain wrong to expect it. So this will not happen.

Denis Grinyuk

2020-04-14 17:08

reporter   ~0122145

Last edited: 2020-04-14 17:10

View 2 revisions

> The basic TJSON object used in the streamer does not guarantee sorted properties.
Could you please kindly specify where is the problem with order of properties stored in TJSONObject?? How can I reproduce it?

At my point of view:
1. TJSONObject uses TFPHashObjectList.Add method adding properties converted to TJSONData. That is TFPHashList.Add in fact. New item is added strictly to the end of hash table.
2. In TJSONObject.AsJSON it goes through TFPHashObjectList.Items one by one in order they were added. And really I always get right fields order in my project.

Michael Van Canneyt

2020-04-14 18:24

administrator   ~0122149

Wait.

I have found a way to do what you want without counting on the hash mechanism internals.

I will add a 'sorted' option to the FormatJSON() options, and sort the elements prior to creating the text.

Then we can add a 'sorted' option to the streamer.

Denis Grinyuk

2020-04-14 21:57

reporter   ~0122150

Last edited: 2020-04-14 21:58

View 2 revisions

But it IS sorted already! Because of TPropInfoList used. Instead I need fields to be the same order as declared in properties.

HashList seems rather straightforward. Items are added to the end of list and taken one-by-one afterwards,

P.S. Thanks a lot for your patience! :-)

Michael Van Canneyt

2020-04-14 22:27

administrator   ~0122151

The currently used hash list is an implementation detail that can be changed at any moment for example for better performance.
So you can *not* count on it that they are stored in the order that they are added.

Now, I initially understood you needed the fields in sorted (alphabetical) order. I think I can deliver that.
If you instead need the JSON properties guaranteed in the order as declared in the class, then I cannot help you, and will simply close the bugreport again.
In that case you better write your own streamer which does what you want.

Denis Grinyuk

2020-04-14 22:43

reporter   ~0122152

Ok. I see. Can we just accept modification for TPropInfoList constructor (to control its items order) and move to some new virtual method (StreamProperties for example) this part of TJSONStreamer.ObjectToJSON ?

      PIL:=TPropInfoList.Create(AObject,tkProperties,False);
      try
        For I:=0 to PIL.Count-1 do
          begin
          PD:=StreamProperty(AObject,PIL.Items[i]);
            If (PD<>Nil) then begin
              if jsoLowerPropertyNames in Options then
                Result.Add(LowerCase(PIL.Items[I]^.Name),PD)
              else
            Result.Add(PIL.Items[I]^.Name,PD);
          end;
          end;
      finally
        FReeAndNil(Pil);
      end;

In this case one can simply inherit from TJSONStreamer.

Michael Van Canneyt

2020-04-18 12:49

administrator   ~0122222

Made a separate CreatePropertyList call, and extracted the streaming of properties to a StreamProperties method. Both are virtual, so you can override them to fit your needs.

Denis Grinyuk

2020-04-19 13:40

reporter   ~0122251

Thanks a lot, Michael!

But what about TPropInfoList? One cannot just override it's constructor as it accesses private vars. The whole class needs to be copied ((
Please include new constructor parameter (Sorted: Boolean = True) from my patch also!

Michael Van Canneyt

2020-05-02 17:50

administrator   ~0122598

Patched TPropInfoList in rev 45222, thanks!

Issue History

Date Modified Username Field Change
2020-03-19 10:31 Denis Grinyuk New Issue
2020-03-19 10:31 Denis Grinyuk File Added: 0001-Fix-for-TJSONStreamer-ignoring-fields-declaration-order.patch
2020-03-19 10:46 Michael Van Canneyt Assigned To => Michael Van Canneyt
2020-03-19 10:46 Michael Van Canneyt Status new => resolved
2020-03-19 10:46 Michael Van Canneyt Resolution open => won't fix
2020-03-19 10:46 Michael Van Canneyt FPCTarget => -
2020-03-19 10:46 Michael Van Canneyt Note Added: 0121646
2020-04-01 12:17 Denis Grinyuk Status resolved => feedback
2020-04-01 12:17 Denis Grinyuk Resolution won't fix => reopened
2020-04-01 12:17 Denis Grinyuk Note Added: 0121826
2020-04-01 12:57 Michael Van Canneyt Status feedback => resolved
2020-04-01 12:57 Michael Van Canneyt Resolution reopened => won't fix
2020-04-01 12:57 Michael Van Canneyt Note Added: 0121830
2020-04-14 17:08 Denis Grinyuk Status resolved => feedback
2020-04-14 17:08 Denis Grinyuk Resolution won't fix => reopened
2020-04-14 17:08 Denis Grinyuk Note Added: 0122145
2020-04-14 17:10 Denis Grinyuk Note Edited: 0122145 View Revisions
2020-04-14 18:16 Michael Van Canneyt Status feedback => resolved
2020-04-14 18:16 Michael Van Canneyt Resolution reopened => won't fix
2020-04-14 18:24 Michael Van Canneyt Status resolved => new
2020-04-14 18:24 Michael Van Canneyt Resolution won't fix => reopened
2020-04-14 18:24 Michael Van Canneyt Note Added: 0122149
2020-04-14 21:57 Denis Grinyuk Note Added: 0122150
2020-04-14 21:58 Denis Grinyuk Note Edited: 0122150 View Revisions
2020-04-14 22:27 Michael Van Canneyt Note Added: 0122151
2020-04-14 22:43 Denis Grinyuk Note Added: 0122152
2020-04-18 12:49 Michael Van Canneyt Status new => resolved
2020-04-18 12:49 Michael Van Canneyt Fixed in Revision => 44776
2020-04-18 12:49 Michael Van Canneyt Note Added: 0122222
2020-04-19 13:40 Denis Grinyuk Status resolved => feedback
2020-04-19 13:40 Denis Grinyuk Note Added: 0122251
2020-05-02 17:50 Michael Van Canneyt Status feedback => resolved
2020-05-02 17:50 Michael Van Canneyt Note Added: 0122598
2020-05-02 17:50 Michael Van Canneyt Resolution reopened => fixed
2020-05-02 17:50 Michael Van Canneyt Description Updated View Revisions