google / json_serializable.dart

Generates utilities to aid in serializing to/from JSON.
https://pub.dev/packages/json_serializable
BSD 3-Clause "New" or "Revised" License
1.56k stars 399 forks source link

Add json allowlist of specific types #1072

Closed rrousselGit closed 2 years ago

rrousselGit commented 2 years ago

Currently, json_serializable does not support custom JSON format which contains classes other than Map/String/num/...

An example would be Firestore, which on top of your typical String/bool/... may also have Timestamp/GeoPoint/DocumentReference as Map values.

Meaning that if we're doing:

@JsonSerializable
class Foo {
  <...>
  final Timestamp timestamp;
}

then json_serializable will complain with the error:

Could not generate `fromJson` code for `timestamp`.
To support the type `Timestamp` you can:
* Use `JsonConverter`
  https://pub.dev/documentation/json_annotation/latest/json_annotation/JsonConverter-class.html
* Use `JsonKey` fields `fromJson` and `toJson`
  https://pub.dev/documentation/json_annotation/latest/json_annotation/JsonKey/fromJson.html
  https://pub.dev/documentation/json_annotation/latest/json_annotation/JsonKey/toJson.html
package:cloud_firestore_odm_generator_integration_test/simple.dart:15:19

The problem is, this error is technically incorrect in this specific use-case since we don't need to encode/decode Timestamp.

We could use JsonConverter/JsonKey, but that is not ideal since they can't be applied to the entire project.

Proposal

We could add a way to globally tell json_serializable to not do anything when encountering certain types.

This could be a configuration inside the build.yaml file, such as:

json_serializable:
  options:
     allowed-types:
         - name: Timestamp
            source: "package:cloud_firestore/cloud_firestore.dart"

Then, when json_serializable will encounter a property typed as Timestamp, the generated code will simply perform as cast instead of trying to call fromJson/toJson.

kevmoo commented 2 years ago

This is an abuse of JSON – but I get it. PR welcome here!

rrousselGit commented 2 years ago

Alternatively, do you believe there's a way to define global type-converters?

We could technically have a TypeConverter simply cast the value. But ideally, it shouldn't require using the converter on every single property matching

atoka93 commented 2 years ago

Was just checking if there are global type-converters, would love that.

kevmoo commented 2 years ago

You can create your own json_serializable package and augment the type_converters provided. The APIs are all public-ish.

atoka93 commented 2 years ago

A bit more code than adding it to the build.yaml but still fairly simple and would solve the issue 🤔 Thanks for the idea @kevmoo!

For anyone reading this who would like an example on how to do that: @knaeckeKami has one here: https://github.com/knaeckeKami/json_serializable_immutable_collections/tree/master/builders/json_serializable_mobx and an explanation regarding it here: https://github.com/knaeckeKami/json_serializable_immutable_collections/issues/15#issuecomment-968073104

knaeckeKami commented 2 years ago

The way I do it in these packages is a bit hacky. I always wished we could specify additional TypeHelpers in the build.yaml, (like ferry does it for example: https://ferrygraphql.com/docs/custom-scalars#configure-custom-serializer ) but this would not be a trivial thing to implement in the existing architecture of json_serializable.

ciriousjoker commented 2 years ago

This is especially relevant now that Firestore ODM is on its way, since it promotes using JsonSerializable to write the models. It would be awesome if the solution for this issue would be a small code snippet that can quickly be added to the getting started instructions over there.

idotalmor commented 2 years ago

I must say I don't like the way the models are created in conjunction with firestore types. that makes the app coupled with firestore which is backend implementation details. please consider alternative solutions.

FXschwartz commented 2 years ago

Any progress on this one? Has an ideal solution been found and we are waiting on implementation?

Running into this using Firestore ODM

ciriousjoker commented 2 years ago

@idotalmor Not necessarily. Just having an annotation that says "don't convert this field" would be enough to support Firestore and no new dependencies would be required.

kevmoo commented 2 years ago

I could just put bool ignoreType field on JsonKey – would that work?

ciriousjoker commented 2 years ago

@kevmoo Assuming it still includes the property in the generated class, yes that should be enough.

kevmoo commented 2 years ago

@kevmoo Assuming it still includes the property in the generated class, yes that should be enough.

The really tricking thing will be if the type is used like List<Map<String, WeirdObject>> – will need to ponder that...

FXschwartz commented 2 years ago

Is there an example on how to use JsonConverter/JsonKey to handle this as a workaround?

knaeckeKami commented 2 years ago

I implemented a prototype of how this can be solved with a custom builder here.

The ingredients to make this work:

  1. custom typehelper that receives a set of types that should be ignored: https://github.com/knaeckeKami/json_serializable_immutable_collections/blob/allowlist/typehelpers/json_serializable_type_helper_utils/lib/src/fake_json_typehelper.dart

  2. a builder.dart file that declares a new json_serializable builder with that typehelper added: https://github.com/knaeckeKami/json_serializable_immutable_collections/blob/allowlist/fake_json_example/lib/builder.dart#L34

  3. a build.yaml file that disables the default builder of json_serialzable and adds our custom one: https://github.com/knaeckeKami/json_serializable_immutable_collections/blob/allowlist/fake_json_example/build.yaml

I did not really test this, and I did not think about inheritance/subtyping, so YMMV. But this should be able to work around the issue.

rrousselGit commented 2 years ago

The really tricking thing will be if the type is used like List<Map<String, WeirdObject>> – will need to ponder that...

If that's about having allowList: [WeirdObject] and having json_serializable support List<WeirdObject>, then I think we can keep the current behavior for those cases (aka having the code-generator fail)

These can be supported late down the road if useful. I doubt these cases will be necessary


Thinking about this more, instead of an allowlist what about:

class MyJsonConverter extends JsonConverter<NotSerializable> {}

@JsonSerializable(converters: [MyJsonConverter()])
class Example {
  NotSerializable first;
  NotSerializable second;
}

Then if folks want to apply the converters often, they can simply make a custom annotation:

const firebaseSerializable = JsonSerializable(converters: [DocumentReferenceConverter(), <some more>]);

@firebaseSerializable
class Example {
  final DocumentReference ref; // OK
}