Sideloading Wrapper for the Attributes/JSON Adapters

[NullVoxPopuli]

Related: #1084 #1107 #1088

So, in order to preserve functionality implemented in the attributes/json adapters, I decided to still use the attributes adapter to handle the rendering of the hash/json (so we get all the features (include, etc)).

It is a little more overhead than sideloading while iterating over the associations, but I think this could be the least likely to produce bugs, as it relies on pre-existing, thoroughly tested code.

the purpose is for those who for some reason can't use json-api, and need sideloaded data (for example, in an ember app - which doesn't like nested relationships)

[NullVoxPopuli]

are there built in helpers for these?

[beauby]

This config should be namespaced, as it is specific to this adapter.

[NullVoxPopuli]

ah yes, this was left over from the last implementation. thanks

[bf4]

What's the purpose of this adapter and where / how is it used? I can't tell. Also, what does the name mean?

[NullVoxPopuli]

@bf4 the purpose is for those who for some reason can't use json-api, and need sideloaded data (for example, in an ember app - which doesn't like nested relationships)

I called it flat json, because there should be no nested json objects representing models in the result -- the originally nested json is _flat_tened. idk. I'm not in love with the name.

[NullVoxPopuli]

@byteg glad you find this helpful!

Unfortunately, I don't know the best way to go about only including the ids for an association.
This would require some team discussion.

And then, I don't know how nested relationships would work when only using ids. But maybe there are some things from json api that we could borrow from for this?

[byteg]

@NullVoxPopuli i could solve the problem with that simple workaround:

class PostSerializer < ActiveModel::Serializer
attributes :id, :title, :content, :comment_ids

def comment_ids
object.comments.collect(&:id)
end
end
But it fails with error :(

undefined method `each_with_object' for 1:Fixnum

If it's possible to fix that, all the problems are solved for now :)

[vasilakisfil]

I am pretty sure Ember supports nested relationships. Which ember adapter/serializer do you use ? In any case it's good to have more adapters given that they are maintained of course.

[vasilakisfil]

@NullVoxPopuli

[NullVoxPopuli]

@vasilakisfil I use the ActiveModelAdapter.
I'll switch to json-api at some point, though

[NullVoxPopuli]

@byteg I just pushed something that fixes your issue.

However, as I continue to work on this, I begin to be more and more in favor of just having people switch to json-api.

I'll be making this change myself in the coming months, whenever I can find time for it.

[vasilakisfil]

Is this still valid? Is the core team interested to merge it? Would be nice cause it would provide compatibility with 0.8.x and 0.9.x adapters.

[erlandsona]

Any updates on how 0.10.x handles sideloading the association ids with the json adapter? I tried the config.sideload_associations but that didn't work? And I'm getting undefined method embed because it seems this has been an ongoing debate for a while now. Just curious about the latest?

[NullVoxPopuli]

@erlandsona does using this branch in your gemfile help your problem?

gem 'active_model_serializers', github: 'NullVoxPopuli/active_model_serializers', branch: 'sideloaded-json'

[vasilakisfil]

@erlandsona unfortunately the core team is not interested to interoperate the 0.8.x/0.9.x adapters in 0.10.x so the only way is to build your own adapter :) @NullVoxPopuli PR is a good start.

@NullVoxPopuli this branch embeds the whole associations not just the ids which I guess @erlandsona wants

[erlandsona]

Gimme a second to try @NullVoxPopuli and see if that works...

[NullVoxPopuli]

@vasilakisfil we'd just like everyone to upgrade, but we understand that - that isn't a possibility with some people, and that's why everything is (or is trying to be) architected to be pretty flexible. .8 and .9 compatibility can be added to this adapter. -- I'm no expert on the older versions, though -- so I'm not sure how much hackery would need to be done to achieve full 0.8 / 0.9 compatibility.

[vasilakisfil]

Sure I can work on fixining the compatibility, but is the core interested to merge the new adapter in the master or will it be yet another pull request hanging around ?

[vasilakisfil]

Because saying to the user use gem 'active_model_serializers', github: 'NullVoxPopuli/active_model_serializers', branch: 'sideloaded-json' is not very helpful for production env. What if you delete your repo ? :)

[erlandsona]

@NullVoxPopuli I just tried you're version with the config.sideload_associations option set to true but it's still giving me the entire record. Here's the output and setup...

Output (json) localhost:3000/brands (shortened)

{
  "brands": [
    {
      "id": 479,
      "name": "37075",
      "models": []
    },
    {
      "id": 540,
      "name": "A.seefeldt",
      "models": [
        {
          "id": 1859,
          "name": "S1008"
        }
      ]
    },
    {
      "id": 501,
      "name": "Adamovic",
      "models": [
        {
          "id": 1842,
          "name": "5 String Bass"
        }
      ]
    }
  ]
}

localhost:3000/models (shortened)

{
  "models": [
    {
      "id": 1855,
      "name": "'61 Reissue Strat",
      "brands": [
        {
          "id": 533,
          "name": "Svl"
        }
      ],
      "sub_models": []
    },
    {
      "id": 946,
      "name": "0",
      "brands": [
        {
          "id": 40,
          "name": "Martin"
        }
      ],
      "sub_models": []
    },
    {
      "id": 663,
      "name": "0 9",
      "brands": [
        {
          "id": 29,
          "name": "Larrivée"
        }
      ],
      "sub_models": []
    }
  ]
}

Here's some more info on the setup...
active_model_serializer.rb (initializer)

ActiveModel::Serializer.config.adapter = :json
ActiveModel::Serializer.config.sideload_associations = true

brand_serializer.rb

class BrandSerializer < ActiveModel::Serializer
  attributes :id, :name
  has_many :models
end

model_serializer.rb

class ModelSerializer < ActiveModel::Serializer
  attributes :id, :name
  has_many :brands
  has_many :sub_models
end

sub_model_serializer.rb

class SubModelSerializer < ActiveModel::Serializer
  attributes :id, :name
  has_many :models
end

brands_controller.rb

      def index
        @brands = policy_scope(Brand)
          .filter_by_id(params[:ids])
          .filter_by_name(params[:name])
          .filter_by_approved(params.has_key?(:approved))
          .filter_by_pending(params.has_key?(:pending))
          .page(params[:page])
          .per(params[:per_page])

        # render json: Oj.dump(json_for(@brands), mode: :compat)
        render json: @brands
      end

A few things I'm trying to get to work are...

I'd like only to include the id's of the associations because I'm getting the infinite loop with the HABTM circular reference that happens when I try to get brands from models and brands gets models which gets brands etc...

I also want to utilize if possible Rails include to get rid of the N+1 issue.

I'm also trying to figure out how to get the Oj gem to play nicely with AMS so when I ask for per_page=2500 it doesn't take 5+ seconds to load the json response??? Can't quite figure that out either.

If anyone has anytime to set up a google hangout to figure this out I would greatly appreciate the help!

[NullVoxPopuli]

are you setting the adapter to :flat_json?

[vasilakisfil]

@erlandsona what you ask for (including only ids) is not possible in this adapter. But yea you should set it to :flat_json :)

[vasilakisfil]

:O did you embed the ids? cause I was trying that last week and wasn't
working

I could say it's almost 0.9.x compliant then
On Oct 23, 2015 2:42 AM, "Austin Erlandson" [email protected]
wrote:

Got it hey that worked! That's exactly what I was looking for. Thank you
so much! Why again hasn't that been added to the main repo? It makes the
JSON super lean, and flexible? I know the json_api spec does essentially
the same thing but this is such a small project currently that the json_api
spec seemed a bit overkill for our use case.

Any thoughts on the N+1 issue? That may be a different issue... I tried
setting the default_scopes on the models like so...

class Brand < ActiveRecord::Base
has_and_belongs_to_many :models
default_scope { includes(:models).order(name: :asc) }
end

class Model < ActiveRecord::Base
has_and_belongs_to_many :brands
has_and_belongs_to_many :sub_models
default_scope { includes(:brands, :sub_models).order(name: :asc) }
end

class SubModel < ActiveRecord::Base
has_and_belongs_to_many :models
default_scope { includes(:models).order(name: :asc) }
end

Obviously this leads to an infinite loop but it does solve the N+1 issue
haha?

—
Reply to this email directly or view it on GitHub
#1198 (comment)
.

[NullVoxPopuli]

@erlandsona in your controller, you could specify the include option, which isn't yet documented, but there is an issue to get it done.

basically, using include: '*' only allows first-order associations. give it a go. :-)

you can also manually specify which associations you want, like: include: [author: :blog, :comments]

[erlandsona]

@NullVoxPopuli I've come across that reading through all these issues and there's a couple places where people define the includes piece and I just tried to add it alone and with the includes calls in the models but without the includes calls in the models I'm still getting N+1 and with them I get infinite loop. I'm probably doing it wrong though...

here's the server output with the includes calls in the models

  Client Load (1.3ms)  SELECT  "clients".* FROM "clients" WHERE "clients"."auth_token" = $1  ORDER BY "clients"."name" ASC LIMIT 1  [["auth_token", "I'm a SecureRandom hash"]]
  Model Load (1.5ms)  SELECT  "models".* FROM "models"  ORDER BY "models"."name" ASC LIMIT 25 OFFSET 0
  HABTM_Brands Load (1.8ms)  SELECT "brands_models".* FROM "brands_models" WHERE "brands_models"."model_id" IN (1855, 946, 663, 947, 948, 949, 951, 950, 952, 953, 954, 956, 955, 1186, 957, 1164, 1165, 1460, 958, 959, 960, 961, 962, 963, 964)
  Brand Load (2.2ms)  SELECT "brands".* FROM "brands" WHERE "brands"."id" IN (533, 40, 29, 47, 43, 110)  ORDER BY "brands"."name" ASC
  HABTM_Models Load (3.4ms)  SELECT "brands_models".* FROM "brands_models" WHERE "brands_models"."brand_id" IN (43, 47, 29, 40, 110, 533)
  Model Load (8.1ms)  SELECT "models".* FROM "models" WHERE "models"."id" IN (1164, 1165, 1166, 1167, 1168, 1169, 1170, 1171, 1172, 1173, 1174, 1175, 1176, 1177, 1178, 1179, 1180, 1181, 1186, 1187, 1188, 1189, 1190, 1191, 1192, 1193, 1194, 1195, 1196, 1197, 1198, 1199, 1200, 1201, 1202, 1203, 1204, 1205, 1206, 1207, 1208, 1209, 1210, 1211, 1212, 1213, 1214, 1215, 1216, 1217, 1218, 1219, 1220, 1221, 1222, 1223, 1224, 1225, 1226, 1227, 663, 664, 665, 666, 667, 668, 669, 670, 671, 672, 673, 674, 675, 676, 677, 678, 679, 680, 681, 946, 947, 948, 949, 950, 951, 952, 953, 954, 955, 956, 957, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 982, 983, 984, 985, 986, 987, 988, 989, 990, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, 1008, 1009, 1010, 1011, 1012, 1013, 1014, 1015, 1016, 1017, 1018, 1019, 1020, 1021, 1022, 1023, 1024, 1025, 1026, 1027, 1028, 1029, 1030, 1031, 1032, 1033, 1034, 1035, 1036, 1037, 1038, 1039, 1040, 1041, 1042, 1043, 1044, 1045, 1046, 1047, 1048, 1049, 1050, 1051, 1052, 1053, 1054, 1055, 1056, 1057, 1058, 1059, 1060, 1061, 1062, 1063, 1064, 1065, 1066, 1067, 1068, 1069, 1070, 1071, 1072, 1073, 1074, 1075, 1076, 1077, 1078, 1079, 1080, 1081, 1082, 1083, 1084, 1085, 1086, 1087, 1088, 1089, 1090, 1091, 1092, 1093, 1094, 1095, 1096, 1097, 1098, 1099, 1100, 1101, 1102, 1103, 1104, 1105, 1106, 1107, 1108, 1109, 1110, 1111, 1112, 1113, 1114, 1115, 1116, 1117, 1118, 1119, 1120, 1121, 1122, 1123, 1124, 1125, 1126, 1127, 1128, 1129, 1130, 1131, 1132, 1133, 1134, 1135, 1136, 1137, 1138, 1139, 1140, 1141, 1142, 1143, 1144, 1460, 1855)  ORDER BY "models"."name" ASC
  HABTM_Brands Load (4.3ms)  SELECT "brands_models".* FROM "brands_models" WHERE "brands_models"."model_id" IN (1855, 946, 663, 947, 948, 949, 951, 950, 952, 953, 954, 956, 955, 1186, 957, 1164, 1165, 1460, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 983, 982, 985, 984, 986, 987, 988, 1187, 1188, 989, 990, 664, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1189, 1000, 665, 1001, 1002, 1003, 1190, 1191, 666, 667, 1166, 1167, 1168, 1004, 1005, 1169, 1192, 1006, 668, 669, 1193, 1194, 1195, 1008, 1007, 1196, 1198, 1197, 1199, 1200, 1201, 1009, 1010, 1202, 1170, 1171, 1011, 1203, 670, 671, 672, 1012, 1013, 1014, 1015, 1016, 1017, 1018, 1172, 1019, 1020, 1021, 1022, 1023, 1024, 1026, 1025, 1027, 1028, 1029, 1030, 1031, 1032, 1033, 1204, 1034, 1035, 1036, 1205, 1037, 1038, 1039, 1040, 1041, 1207, 1206, 1042, 1043, 1208, 1209, 1210, 1211, 1212, 1044, 1045, 1046, 1047, 1048, 1049, 1050, 1051, 1052, 1053, 1054, 1055, 1056, 1213, 1057, 1214, 1058, 1059, 1060, 1173, 1061, 1062, 673, 1063, 1064, 1065, 1066, 1067, 1068, 1074, 1069, 1070, 1071, 1072, 1073, 1075, 1076, 1077, 1078, 1079, 1081, 1080, 1082, 1083, 1084, 1085, 1086, 1087, 1215, 674, 1088, 1089, 1090, 1091, 1092, 1093, 1094, 1095, 675, 676, 1096, 677, 678, 1097, 1098, 1099, 1100, 1101, 1216, 1102, 1103, 1217, 1104, 1105, 1218, 1174, 1219, 1106, 1107, 1108, 679, 1220, 1109, 1110, 1111, 1112, 1113, 1221, 1114, 1115, 1116, 680, 1222, 1223, 1117, 1118, 1224, 1225, 1119, 1120, 1121, 1122, 1123, 1124, 1125, 1126, 1175, 1226, 1127, 1128, 1129, 1130, 1131, 1132, 1227, 1176, 1133, 1134, 1135, 1136, 1137, 1138, 681, 1139, 1177, 1178, 1140, 1179, 1180, 1141, 1181, 1142, 1143, 1144)
  CACHE (0.1ms)  SELECT "brands".* FROM "brands" WHERE "brands"."id" IN (533, 40, 29, 47, 43, 110)  ORDER BY "brands"."name" ASC
  CACHE (0.0ms)  SELECT "brands_models".* FROM "brands_models" WHERE "brands_models"."brand_id" IN (43, 47, 29, 40, 110, 533)
  CACHE (0.0ms)  SELECT "models".* FROM "models" WHERE "models"."id" IN (1164, 1165, 1166, 1167, 1168, 1169, 1170, 1171, 1172, 1173, 1174, 1175, 1176, 1177, 1178, 1179, 1180, 1181, 1186, 1187, 1188, 1189, 1190, 1191, 1192, 1193, 1194, 1195, 1196, 1197, 1198, 1199, 1200, 1201, 1202, 1203, 1204, 1205, 1206, 1207, 1208, 1209, 1210, 1211, 1212, 1213, 1214, 1215, 1216, 1217, 1218, 1219, 1220, 1221, 1222, 1223, 1224, 1225, 1226, 1227, 663, 664, 665, 666, 667, 668, 669, 670, 671, 672, 673, 674, 675, 676, 677, 678, 679, 680, 681, 946, 947, 948, 949, 950, 951, 952, 953, 954, 955, 956, 957, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 982, 983, 984, 985, 986, 987, 988, 989, 990, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1000, 1001, 1002, 1003, 1004, 1005, 1006, 1007, 1008, 1009, 1010, 1011, 1012, 1013, 1014, 1015, 1016, 1017, 1018, 1019, 1020, 1021, 1022, 1023, 1024, 1025, 1026, 1027, 1028, 1029, 1030, 1031, 1032, 1033, 1034, 1035, 1036, 1037, 1038, 1039, 1040, 1041, 1042, 1043, 1044, 1045, 1046, 1047, 1048, 1049, 1050, 1051, 1052, 1053, 1054, 1055, 1056, 1057, 1058, 1059, 1060, 1061, 1062, 1063, 1064, 1065, 1066, 1067, 1068, 1069, 1070, 1071, 1072, 1073, 1074, 1075, 1076, 1077, 1078, 1079, 1080, 1081, 1082, 1083, 1084, 1085, 1086, 1087, 1088, 1089, 1090, 1091, 1092, 1093, 1094, 1095, 1096, 1097, 1098, 1099, 1100, 1101, 1102, 1103, 1104, 1105, 1106, 1107, 1108, 1109, 1110, 1111, 1112, 1113, 1114, 1115, 1116, 1117, 1118, 1119, 1120, 1121, 1122, 1123, 1124, 1125, 1126, 1127, 1128, 1129, 1130, 1131, 1132, 1133, 1134, 1135, 1136, 1137, 1138, 1139, 1140, 1141, 1142, 1143, 1144, 1460, 1855)  ORDER BY "models"."name" ASC
  CACHE (0.0ms)  SELECT "brands_models".* FROM "brands_models" WHERE "brands_models"."model_id" IN (1855, 946, 663, 947, 948, 949, 951, 950, 952, 953, 954, 956, 955, 1186, 957, 1164, 1165, 1460, 958, 959, 960, 961, 962, 963, 964, 965, 966, 967, 968, 969, 970, 971, 972, 973, 974, 975, 976, 977, 978, 979, 980, 981, 983, 982, 985, 984, 986, 987, 988, 1187, 1188, 989, 990, 664, 991, 992, 993, 994, 995, 996, 997, 998, 999, 1189, 1000, 665, 1001, 1002, 1003, 1190, 1191, 666, 667, 1166, 1167, 1168, 1004, 1005, 1169, 1192, 1006, 668, 669, 1193, 1194, 1195, 1008, 1007, 1196, 1198, 1197, 1199, 1200, 1201, 1009, 1010, 1202, 1170, 1171, 1011, 1203, 670, 671, 672, 1012, 1013, 1014, 1015, 1016, 1017, 1018, 1172, 1019, 1020, 1021, 1022, 1023, 1024, 1026, 1025, 1027, 1028, 1029, 1030, 1031, 1032, 1033, 1204, 1034, 1035, 1036, 1205, 1037, 1038, 1039, 1040, 1041, 1207, 1206, 1042, 1043, 1208, 1209, 1210, 1211, 1212, 1044, 1045, 1046, 1047, 1048, 1049, 1050, 1051, 1052, 1053, 1054, 1055, 1056, 1213, 1057, 1214, 1058, 1059, 1060, 1173, 1061, 1062, 673, 1063, 1064, 1065, 1066, 1067, 1068, 1074, 1069, 1070, 1071, 1072, 1073, 1075, 1076, 1077, 1078, 1079, 1081, 1080, 1082, 1083, 1084, 1085, 1086, 1087, 1215, 674, 1088, 1089, 1090, 1091, 1092, 1093, 1094, 1095, 675, 676, 1096, 677, 678, 1097, 1098, 1099, 1100, 1101, 1216, 1102, 1103, 1217, 1104, 1105, 1218, 1174, 1219, 1106, 1107, 1108, 679, 1220, 1109, 1110, 1111, 1112, 1113, 1221, 1114, 1115, 1116, 680, 1222, 1223, 1117, 1118, 1224, 1225, 1119, 1120, 1121, 1122, 1123, 1124, 1125, 1126, 1175, 1226, 1127, 1128, 1129, 1130, 1131, 1132, 1227, 1176, 1133, 1134, 1135, 1136, 1137, 1138, 681, 1139, 1177, 1178, 1140, 1179, 1180, 1141, 1181, 1142, 1143, 1144)

[erlandsona]

@vasilakisfil You're right that 0.9.x is set up this way but with most issues I've read about the main contributors basing the design of 0.10.x on 0.8.x I didn't want to commit myself to a version that wasn't going to be well supported in the future? I don't know maybe that's wise or not, I'm just trying to get all this stuff to just work.

[NullVoxPopuli]

@erlandsona it's possible then that this adapter will need re-doing a bit, and actually won't be able to inherit from the attributes adapter.

cause it's the attributes adapter that's causing the infinite loop.

if the sideloading only adapter didn't inherit from attributes, it could just skip over models that have already been prepped for serialization.

[erlandsona]

Hmm... little over my head there. I just want to be sure you're clear that the infinite loop only happens when I specify the includes calls in the models. I'm not actually sure it's an AMS issue or whether it's just a straight up Rails issue? But I'm not finding anything useful on SO about HABTM with includes going both directions?

That said, your inclination is that the adapter is the issue here? What I'm really asking is whether or not I screwed up somewhere? I'm just trying to get json where...

{
  brands: [
    {
      id: 1,
      name: "name1",
      models: [1,3,21,48, etc...]
    },
    {
      id: 2,
      name: "name2",
      models: [2,4,22,37, etc...]
    }
  ]
}

and for /models

{
  models: [
    {
      id: 1,
      name: "model_name1",
      brands: [1,3, 5, 7, etc...],
      sub_models: [1,3,5,7, etc...]
    },
    {
      id: 2,
      name: "model_name2",
      brands: [2,4,6,8, etc...],
      sub_models: [2,4,6,8, etc...]
    }
  ]
}

I really don't want to have to look into other Serializer gems because I keep reading about how jBuilder is too slow or too complex and AMS just has this really nice PORO interface that I enjoy but I'm just trying to get it to do what I want?

[NullVoxPopuli]

I think the problem is happening because of the circular association definition in the models.

However, in an adapter, it's possible to evade infinite loops by keeping track of which models have already been prepped for serialization (and this would be the fastest way to side load).

Currently, the whole json object is 'rendered' in the attributes adapter, and then the FlatJson adapter parses the output, and rearranges everything in to the nifty sideloadedness.

The solution here would be for me to change the implementation of this adapter.

Which, I can do tomorrow morning. :-)

[erlandsona]

Hmmm! Alrighty then. I'll look forward to seeing the update! Is this issue the best place to keep in touch or where else should I check in?

[NullVoxPopuli]

yeah, this is a good spot. :-)

[erlandsona]

Alright, sounds good... Not to be buggy but should I follow
https://github.com/rails-api/active_model_serializers/blob/master/docs/howto/add_pagination_links.md

for adding pagination links with the :flat_json adapter too? Or do you have any other thoughts?

[NullVoxPopuli]

most of the json-api-specific features aren't planned for the other adapters, as far as I know.

I'd first perfer a re-architecting all adapter are to share features.

[erlandsona]

Yeah that sounds nice!

[NullVoxPopuli]

possibly a cleaner way to do this would be to just unwrap the json-api produced hash - that way all the features with json api (the most actively developed adapter) would be present in the sideloaded json adapter

[NullVoxPopuli]

@erlandsona I just made some changes to this branch which implement @beauby 's deserialization (with some modifications).

Can you see if that meets your needs?

[NullVoxPopuli]

@beauby these are the types of inline comments I talk about when I ask you to add comments.

just real informal explanation behind the decision of the implementation

[beauby]

I don't like those, they clutter my screen and I can't see the code :)

[NullVoxPopuli]

it's very helpful for people seeing the code for the first time though. and I think it's worth it. It reduces the required amount of retained control flow in a person's short term memory, which leads to quicker changes.

[vasilakisfil]

As someone who has looked a lot in the code of this project and monkey patched some of it to adapt in my needs I have to admit such comments would help me a lot + would make me confident to brag about my changes and send pull requests back. Now I feel like I have found a way to "make it work" how I want it but there might be a better way because I don't have a clue what the original maintainer meant/wanted to achieve on some parts. Ruby is awesome but sometimes it's not self explained like other languages :)

[NullVoxPopuli]

yes, exactly. :-)

[beauby]

To be honest, I'm all up for comments stating intents, but still against comments demystifying bad code (which should instead be rewritten into good code).

[NullVoxPopuli]

I totally agree with your intention there. No amount of comments can compensate for bad code.

But even good code needs narration.

[bf4]

@beauby I'm pro @NullVoxPopuli and @vasilakisfil. If the code isn't self-documenting, it needs documentation. Comments help tremendously. Ideal could is self documenting, real code, whether good or bad, may benefit from comments explaining why the code is the way it is, what decisions were made, and why. When I read code, I don't want to have to pull my hair out trying to figure out what it does. So, major props to @NullVoxPopuli for pointing out this example

[bf4]

OTOH, classic tweet:

Code Comments

[erlandsona]

@NullVoxPopuli Sorry I've been absent from this thread... I've since moved onto just overriding as_json in the model layer to make things compatible with the Oj gem... That said, this project is also currently undergoing an overhaul... Given the current state of my project... I'm foreseeing the eventual need to come back to AMS.

That said I fixed the N+1 issue I was having with the old structure and realized I didn't actually need the nested association reference id's in the JSON output.

My current issue is trying to get AMS to play nicely with the Oj gem... basically the main issue is that there's like four or five different definitions of the as_json / to_json methods going on between the two gems and rails... so I can't get a handle on what's happing where and when to make sure that I'm properly integrating both gems...

Any thoughts from the AMS community?

[NullVoxPopuli]

do you have a link to the Oj gem? I haven't heard of it.

[bf4]

obviously the puts has to go :)... maybe use the logger here for your purposes?

[NullVoxPopuli]

:-)

yeah, I don't think I'm going to make any updates to this until the deserialization PR gets merged.
Then I can apply the suggestions @beauby had

[NullVoxPopuli]

I think I'm going to close this, as I want to support the push to JSON API, and not try to mimic its functionality in less-strict adapters.