Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add json allowlist of specific types #1072

Closed
rrousselGit opened this issue Dec 17, 2021 · 16 comments · Fixed by #1135
Closed

Add json allowlist of specific types #1072

rrousselGit opened this issue Dec 17, 2021 · 16 comments · Fixed by #1135

Comments

@rrousselGit
Copy link
Contributor

rrousselGit commented Dec 17, 2021

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
Copy link
Collaborator

kevmoo commented Dec 17, 2021

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

@rrousselGit
Copy link
Contributor Author

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
Copy link

atoka93 commented Dec 30, 2021

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

@kevmoo
Copy link
Collaborator

kevmoo commented Dec 30, 2021

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

@atoka93
Copy link

atoka93 commented Dec 31, 2021

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:
jperezr21/json_serializable_immutable_collections#15 (comment)

@knaeckeKami
Copy link

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
Copy link

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
Copy link

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
Copy link

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

Running into this using Firestore ODM

@ciriousjoker
Copy link

@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
Copy link
Collaborator

kevmoo commented Feb 7, 2022

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

@ciriousjoker
Copy link

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

@kevmoo
Copy link
Collaborator

kevmoo commented Feb 7, 2022

@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
Copy link

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

@knaeckeKami
Copy link

knaeckeKami commented Feb 8, 2022

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
Copy link
Contributor Author

rrousselGit commented Apr 21, 2022

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
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants