[dart-dio] handle polymorphism + discriminator serialization

[ahmednfwela]

This PR aims to add polymorphism support to dart-dio

TODO:

• [x] handle discriminator-based serialization/deserialization in schema with mapping specified - (how to handle it without mapping ?)
• [x] change allOf handling to implement base class/classes instead of duplicating the members
• [x] fix oneOf/anyOf support (done via https://pub.dev/packages/one_of)
• [x] write tests for polymorphism

PR checklist

• [x] Read the contribution guidelines.
• [x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• [x] Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh bin/configs/dart*
  ./bin/utils/export_docs_generators.sh
  

• [x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@jaumard (2018/09) @josh-burton (2019/12) @amondnet (2019/12) @sbu-WBT (2020/12) @kuhnroyal (2020/12) @agilob (2020/12) @ahmednfwela (2021/08)

[ahmednfwela]

ok all checks are passing, gonna need more test schemas for polymorphism, are there any good ones ? @wing328

[kuhnroyal]

Would love to see support for this. Can you make a separate PR which adds the plain @BuiltValue() annotation uses the StructuredSerializer so that we have less noise in this PR?

[ahmednfwela]

why make separate PRs tho? it will take more time to merge and sync both than just one PR

[kuhnroyal]

it will take more time to merge and sync both than just one PR

It may seem that way but the actual changes are way easier to trace and identify. You can also base the PRs on each other.

[ahmednfwela]

but the plain BuiltValue() is part of this PR so that built_value can detect classes when they don't implement Built interface also StructuredSerializer casting can be put in the serializer logic anyway, which i am working on right now

[ahmednfwela]

I have also stumbled on something interesting, in csharp-netcore the generator doesn't support having properties in addition to oneOf/anyOf relevant links: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/csharp-netcore/model.mustache#L44 https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/csharp-netcore/modelOneOf.mustache https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/csharp-netcore/modelAnyOf.mustache but I have found schemas where properties are combined with oneOf/anyOf: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/3_0/anyOf.yaml https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/test/resources/3_0/oneOf.yaml

was that just not considered during implementation, or is the case itself illegal? @wing328 @mandrean @frankyjuang @shibayan @Blackclaws @lucamazzanti

also if it was legal, what would that case exactly mean?

[ahmednfwela]

I rewrote the serializers and the class definition
I also moved the oneOf serialization handling to a separate package complete with its own tests here: https://github.com/Bdaya-Dev/dart-oneof/blob/main/one_of_serializer_tests/test/one_of_serializer_tests.dart https://pub.dev/packages/one_of_serializer right now, it would be great if someone helped with writing tests but generally i think PR is ready for review

[kuhnroyal]

We should not force this into 6.0.0, i'll see if i can get some tests up in the next few days. Can we get rid of the quiver package as a dependency?

Why the change to PrimitiveSerializer for complex objects?

[ahmednfwela]

the change to PrimitiveSerializer is to handle oneOf primitives (e.g. oneOf num, SomeComplexObject)

and I tested that returning a List<Object?> from a PrimitiveSerializer gives it the same behavior as a StructuredSerializer

also I only need quiver and collection for ListEquality, MapEquality and hash functions

also this PR doesn't contain any breaking charges, so why shouldn't we add it to v6?

[banool]

How does this PR compare to https://github.com/OpenAPITools/openapi-generator/pull/10313 btw?

[ahmednfwela]

@banool they are nothing alike to be honest, my PR adds support for oneOf, anyOf, inheritance and discriminator lookup

the other PR just fixes the name generation and uses dynamic to represent oneOf

[banool]

Thanks sounds good. I just used this branch to great success in https://github.com/banool/aptos_api_dart, solving a lot of my woes with the existing generator, thanks a lot for the good work here. Here's hoping this can get accepted into main soon.

[banool]

I have noticed that some types that were formerly concrete are now abstract. For example:

https://github.com/banool/aptos_api_dart/blob/main/lib/src/model/user_transaction_request.dart#L25

Which comes from:

https://github.com/aptos-labs/aptos-core/blob/main/api/doc/openapi.yaml#L1051

As you can see, this doesn't use allOf, so I'm not sure why this is different / I don't know how to instantiate an instance of this now. Indeed it says instantiatable: false, but I'm not sure why. Is this working as intended? It seems like if an object in the spec is used both free standing as well as one of the things used in allOf, it incorrectly assumes it is uninstantiatable.

You can see how I'm using the generator in https://github.com/banool/aptos_api_dart:

• generate_from_openapi.sh
• openapi-generator.yaml

Update: Yeah the more I look at the spec and what is generated from it, the more I'm convinced something isn't quite right. To build a SubmitTransactionRequest, you need a UserTransactionSignature (and a UserTransactionRequest), which requires a TransactionSignature, which requires a Ed25519Signature, which requires a signature, and that signature can only come from calling /transactions/signing_message, which requires a UserTransactionSignature. As far as I can tell this is sort of the "entrypoint" of this flow, so it needs to be possible to instantiate this object directly, not just as part of another object.

[ahmednfwela]

it's because these schemas are parents to other schemas

the basic idea is this: abstract class A implements B, C, Built abstract class B abstract class C

means B, C are parents and can't be instantiated A is a child and can be instantiated

and since dart doesn't support overloading or explicit interface members like c#, you can't have an object implement multiple Built interfaces, so you can't have

abstract class A implements B, C, Built abstract class B implements Built

since now A implements two different Built signatures

so how to work around this? introduce another class for every parent class

so you would have abstract class A implements B, C, Built abstract class B abstract class $B implements B, Built abstract class C abstract class $C implements C, Built

this is what lead me to discover this issue https://github.com/google/built_value.dart/issues/1143

this is just the tradeoff we have to take to support inheritance

[banool]

I see, so we choose to make the abstract version of the class UserTransactionRequestBuilder, which is used to help define child classes, and if I want a concrete version of this, I use $UserTransactionRequestBuilder. Seems like a reasonable tradeoff. Is it possible to mention this in https://github.com/OpenAPITools/openapi-generator/blob/master/docs/generators/dart-dio.md so it is more obvious to folks using the new version of the library. As it is now this isn't easily understandable on first glance.

[ahmednfwela]

you are right, I definitely need to document this better. and thinking about it now, this actually is a breaking change

it might be better to add a flag for users to opt in this feature to avoid breaking changes

[banool]

I see we already have a flag that with your change doesn't do anything, legacyDiscriminatorBehavior. By default this is true, perhaps if the user sets this to false it opts in to your new behavior? On the other hand, this could be part of the breaking changes landing for v6.

[ahmednfwela]

tbh, it would be great to leave this as the default behavior even if it is a breaking change, since the old behavior was very annoying for me

[banool]

Agreed, not sure what the other maintainers think but this feels like an appropriate time to make the change since we just moved to v6 and renamed dart-dio-next to dart-dio.

[banool]

Another question @ahmednfwela, do you have examples showing how to use the OneOf functionality? I'm currently trying to do something like this:

    ScriptFunctionPayloadBuilder scriptFunctionPayloadBuilder =
        ScriptFunctionPayloadBuilder()
          ..type = "script_function_payload"
          ..function_ = "0x1::Coin::transfer"
          ..typeArguments = ListBuilder(["0x1::TestCoin::TestCoin"])
          ..arguments = ListBuilder([
            StringJsonObject(otherAddress.withPrefix()),
            StringJsonObject("100")
          ]);
          
    TransactionPayloadBuilder transactionPayloadBuilder =
        TransactionPayloadBuilder()
          ..oneOf = OneOf4<ScriptFunctionPayload, ScriptPayload,
                  ModuleBundlePayload, WriteSetPayload>(
              value: scriptFunctionPayloadBuilder, typeIndex: 0);

But I'm getting a stack overflow (https://gist.github.com/banool/e62d83ec190ef5017da9bb6a92d318af) when finally using the client to make a request with an object that contains this object, so I imagine I'm doing it wrong. The type in question has 4 options (https://github.com/aptos-labs/aptos-core/blob/main/api/doc/openapi.yaml#L1232), so I've tried both the above and OneOf1.

I've also tried changing the order of the type parameters to match the order in discriminatorMapping and I've also tried passing in the built type vs the builder.

If you have such examples, those would also be good to include in some docs somewhere.

Also I don't believe it should be possible for this code to compile and then fail at runtime as a general principle.

[kuhnroyal]

I am not sure about the timetable for the 6.0.0 release. Lokking at the sample, I wonder if we could use a mixin for the parent fields and create an abstract class $Animal(Parent) additionally to a class Animal to avoid the breaking change.

[ahmednfwela]

@banool you should pass the built type of course, but I am not sure why you are getting a stack overflow, seems like it's trying to serialize the same type over and over again, maybe due to circular reference ? if you can share with me the repo that makes these requests that would be great

@kuhnroyal the problem is that consumers try to de-serialize the base class and expect to get the correct concrete class based on discriminator if we de-serialize a concrete class representing the base class, we won't be able to return the correct concrete class unfortunately this is a limitation of built_value that we have to deal with

[banool]

@ahmednfwela you can repro this by pulling https://github.com/banool/aptos_sdk_dart/ and running flutter test. The origin of the error is here: https://github.com/banool/aptos_sdk_dart/blob/main/lib/aptos_client_helper.dart#L77. I don't believe I have a circular reference but I might be mistaken.

[banool]

Thanks a lot for the PR! I wonder, shouldn't the code generator do this for me? Also it doesn't seem to fix the issue, I still get the circular serialisation issue. Perhaps I also need to pass the serialisers into my Dio, but an early attempt at that didn't work, so perhaps not.

[banool]

Seems like there are issues on the deserializer side of the world too.

Repro:

1. Pull latest version of https://github.com/banool/aptos_sdk_dart
2. Run flutter test
3. Observe the following error:

00:02 +6 -1: /Users/dport/github/aptos_sdk_dart/test/full_library_test.dart: test full transaction flow [E]
  DioError [DioErrorType.other]: Deserializing '[type, pending_transaction, hash, 0x514bee4c00e67c8b143ea6af6227a65805c0ca7de...' to 'PendingTransaction' failed due to: Deserializing '[type, script_function_payload, function, 0x1::Coin::transfer, type_arguments...' to 'TransactionPayload' failed due to: Unsupported operation: Couldn't deserialize oneOf for the discriminator value: script_function_payload
  #0      BuiltJsonSerializers._deserialize (package:built_value/src/built_json_serializers.dart:187:11)
  #1      BuiltJsonSerializers.deserialize (package:built_value/src/built_json_serializers.dart:124:18)
  #2      TransactionsApi.submitTransaction (package:aptos_api_dart/src/api/transactions_api.dart:437:36)
  <asynchronous suspension>
  #3      StackZoneSpecification._registerUnaryCallback.<anonymous closure> (package:stack_trace/src/stack_zone_specification.dart:125:47)
  <asynchronous suspension>

  Source stack:
  #0      BuiltJsonSerializers._deserialize (package:built_value/src/built_json_serializers.dart:187:11)
  #1      BuiltJsonSerializers.deserialize (package:built_value/src/built_json_serializers.dart:124:18)
  #2      TransactionsApi.submitTransaction (package:aptos_api_dart/src/api/transactions_api.dart:437:36)
  <asynchronous suspension>
  #3      StackZoneSpecification._registerUnaryCallback.<anonymous closure> (package:stack_trace/src/stack_zone_specification.dart:125:47)
  <asynchronous suspension>

  package:aptos_api_dart/src/api/transactions_api.dart 442:7  TransactionsApi.submitTransaction

If I bypass this and move onto another function, I get a similar error:

Error Type: DioErrorType.other
Error Message: Deserializing '[type, block_metadata_transaction, version, 1309, hash, 0xc102e89bccb2bfb6de0...' to 'Transaction' failed due to: Unsupported operation: Couldn't deserialize oneOf for the discriminator value: block_metadata_transaction
Error Response: {"type":"block_metadata_transaction","version":"1309","hash":"0xc102e89bccb2bfb6de0614d9ccc0836f6175418c0162874a43e5676ac329685f","state_root_hash":"0x8e5190ae6a40ff2fdd506c3c7719a71554fcc7578f6b944e53626551f5ead1d7","event_root_hash":"0x0704f7b28d02e76a4c9f43edf5ddd8dd4def500e4e05f3fdad9be870efcc6978","gas_used":"0","success":true,"vm_status":"Executed successfully","accumulator_root_hash":"0x7c68056068fcd95c8c6c0014d41241ca1eec69b855f25c97bcd44fc66f31d16e","changes":[{"type":"write_resource","address":"0xa550c18","state_key_hash":"0x7e8bea72fc5af8587e37495c9bcbf1a1ed13674251622b4ed373884bf9749972","data":{"type":"0x1::Block::BlockMetadata","data":{"epoch_internal":"86400000000","height":"1309","new_block_events":{"counter":"1309","guid":{"guid":{"id":{"addr":"0xa550c18","creation_num":"5"}},"len_bytes":40}}}}},{"type":"write_resource","address":"0xa550c18","state_key_hash":"0x3ca60dda914f0541ea08b55c9d4b0952e032e54065e1876b5d41cdcff924ae6b","data":{"type":"0x1::Timestamp::CurrentTimeMicroseconds","data":{"microseconds":"1651625975511407"}}}],"id":"0xe8bb67bf27e52621d504f4ba098e62d671d342fae990ec9c933bd945b45171f1","round":"1321","previous_block_votes":["0xfb4d785594a018bd980b4a20556d120c53a3f50b1cff9d5aa2e26eee582a587","0x2b7bce01a6f55e4a863c4822b154021a25588250c762ee01169b6208d6169208","0x43a2c4cefc4725e710dadf423dd9142057208e640c623b27c6bba704380825ab","0x6036270826a1bbf14c6524439fdb32f5b3c9509f84adc4c70203897221b9423f","0x61616c1208b6b3491496370e7783d48426c674bdd7d04ed1a96afe2e4d8a3930","0x66ccccae2058641f136b79792d4d884419437826342ba84dfbbf3e52d8b3fc7d","0x68f04222bd9f8846cda028ea5ba3846a806b04a47e1f1a4f0939f350d713b2eb","0x7a8cee78757dfe0cee3631208cc81f171d27ca6004c63ebae5814e1754a03c79","0x803160c3a2f8e025df5a6e1110163493293dc974cc8abd43d4c1896000f4a1ec","0x83bc902a12eb983a2e8e8fc7a3e0566e462559a275ac9d0e38aca9ab5ba0d8de","0x8901acc589338e73486500df8f0a048d094e2f2e97a2bc40339204d2d1cee2d1","0x969b010b7846e951dfb79a258a7ae82aa617d2f0d09cdd6fe70e8bf1b5f7c03f","0xcd0b9dd27eb3e9c1804d4df1123047d73b6bc012c3e949bcacc549421008c70a","0xcece26ebddbadfcfbc541baddc989fa73b919b82915164bbf77ebd86c7edbc90","0xd21cca0572b563b6af7e56061c74f434e10a3d6ba52ee150c718b37fabf9922c","0xda641d7b1914046c8f7a4b99791db30fd4c4fb4956d899e55289083f45ac5d84","0xdf1a68d5bc7fe8b0bfe6d0f01edac477cbc2eaadd90b937877bfb904ccd08537","0xe7be8996cbdf7db0f64abd17aa0968074b32e4b0df6560328921470e09fd608b"],"proposer":"0x6bbf2564ea4a6968df450da786b40b3f56b533a7b700c681c31b3714fc30256b","timestamp":"1651625975511407"}
00:12 +6 -1: /Users/dport/github/aptos_sdk_dart/test/full_library_test.dart: test full transaction flow [E]
  Expected: <true>
    Actual: <false>

  package:test_api                                    expect
  package:flutter_test/src/widget_tester.dart 455:16  expect
  test/full_library_test.dart 82:5                    main.<fn>

Seems like it can't figure out how to deserialize the responses even though I've confirmed they're valid and within the confines of the spec.

[ahmednfwela]

@banool this is an issue with the spec itself, since a discriminator mapping isn't provided, the generator defaults to ScriptFunctionPayload but apparently what's received is script_function_payload

you can add the mapping explicitly, or just omit it entirely, and the de-serializer will brute force until it gets the correct type

[banool]

Ah okay I see what you mean, you mean this: https://github.com/banool/aptos_api_dart/blob/main/lib/src/model/transaction_payload.dart#L39. So our API returns script_function_payload but we don't specify that in the spec? I'm not sure what you mean by omit the mapping entirely but I can look in to adding the mapping.

[ahmednfwela]

@banool you would change in the spec:

TransactionPayload:
  title: Transaction Payload
  oneOf:
    - $ref: '#/components/schemas/ScriptFunctionPayload'
    - $ref: '#/components/schemas/ScriptPayload'
    - $ref: '#/components/schemas/ModuleBundlePayload'
    - $ref: '#/components/schemas/WriteSetPayload'
  discriminator:
    propertyName: type

to just

TransactionPayload:
  title: Transaction Payload
  oneOf:
    - $ref: '#/components/schemas/ScriptFunctionPayload'
    - $ref: '#/components/schemas/ScriptPayload'
    - $ref: '#/components/schemas/ModuleBundlePayload'
    - $ref: '#/components/schemas/WriteSetPayload'

but it's better of course to fix the mapping

[banool]

Thanks a lot, appreciate all the wisdom. I think this PR should be good to go then.

[ahmednfwela]

@kuhnroyal are there any more tests you want to add here or is the PR ready for merge ?