decentralized-identity / did-common-dotnet

MIT License
17 stars 9 forks source link

Serialization and implementation support: System.Text.Json and Json.NET (Newtonsoft) in particular #7

Open veikkoeeva opened 3 years ago

veikkoeeva commented 3 years ago

The core model from https://github.com/decentralized-identity/did-common-dotnet/pull/4 can likely be made agnostic of the current JSON (de)serializer, System.Text.Json (STJ) with low amount of work (removing attributes).

I think first-hand for support for STJ is needed as it is the framework way and its design criteria of security and performance seem to align with this .NET DID (thinking security specifically).

  1. Do we have a known case for using Json.NET where STJ is not supported?
  2. If yes on 1., do we want to provide support for it upfront? Other option currently seem to be that we can maintain that with reasonable effort support can be provded either with slight refactoring on STJ implementation one (Extension Data comes into mind mostly) and/or remove all references to STJ in the core model in any case.

**Edit: I think we need to be (de)serialization agnostic in the sense that CBOR is mentioned in the specification. Even if not, I can easily imagine that sending JSON to IoT devices is not a good idea (part of my day job, even very large installations) and so one would want to (de)serialize the model in some other way. Then if one runs .NET on IoT device, and it may become a case, maybe not referencing STJ in the code makes it easier (I'm not sure about AOT etc.).

It's one case then if a parity, say, with CBOR ought to be provided out-of-the-box.

juanfranblanco commented 3 years ago

Ok moving my frustration from #4 to here https://github.com/decentralized-identity/did-common-dotnet/issues/5#issuecomment-783080103

"I think first-hand for support for STJ is needed as it is the framework way and its design criteria of security and performance seem to align with this."

STJ is new and Newtonsoft is the most widely adopted,

image

Both frameworks brings their own pros and cons, Newtonsoft most compatible with a richer feature set and STJ newer faster but with issues in compatibility.

For end users that will require either option as it stands currently the idea is to have the common layer. Now in an ideal scenario the common layer will use the System.Runtime DataMembers to allow to be fully agnostic of the Json deserialisation implemenation but as it stands this will not be supported after .net 6 (currently we are in .net 5), so that might be in 2 years time.

If the STJ convertors can remove the dependency of the attributes (which obviously is a much harder work) then is a start, then in 2 years time these extensions can be removed at the time.

Other thought was to have interfaces of the model, and each serialiser implement their own model, this is a truly smelly patten, so it is not ideal.

juanfranblanco commented 3 years ago

"2. If yes on 1., do we want to provide support for it upfront? Other option currently seem to be that we can maintain that with reasonable effort support can be provded either with slight refactoring on STJ implementation one (Extension Data comes into mind mostly) and/or remove all references to STJ in the core model in any case."

All the references of STJ in the Common model do not exist anymore, these have been replaced with DataMembers. The lack of support of DataMembers is what is causing a complete failure of deserialization.

Going back to the question, do we want STJ now or can it wait for later?

I truly don't want to bring a full dependency to STJ, as it will force a nuget that is not required and wasm / webgl / aot users don't want to download that extra package if needed,

image

veikkoeeva commented 3 years ago

@juanfranblanco I concur with with a runtime dependency on STJ, it appears I was editing my original post when you replied. To me the number downloads of Json.NET and unquantified compatibility are not a case, though.

So one question now is that do we just remove the attributes and continue working without bringing JSON.NET up to parity with the idea it can be if needed? I am not personally keen to support JSON.NET unless I'm aware of a concrete case where it is needed.

I record here about STJ more explicitly:

I am not sure about ExtensionData, but I think it can be done otherwise. That is more like a "catch all bag" that makes it possible to accept anything by way of an extension point unless on wants to inherit from the core model.

I have not checked why everything fails as the PR got reorgnized and merged and I have not yet pulled it. Maybe that mentioned helps us/someone to sort it out.

juanfranblanco commented 3 years ago

Ok to start with what I have done is to add a new class

 public class JsonCaseNamingPolicy : JsonNamingPolicy
    {
#pragma warning disable CS8764 // Nullability of return type doesn't match overridden member (possibly because of nullability attributes).
        public override string? ConvertName(string name)
#pragma warning restore CS8764 // Nullability of return type doesn't match overridden member (possibly because of nullability attributes).
        {
            if (name.StartsWith("@") && name.Length > 1)
            {
                return "@" + name.Substring(1, 1).ToLower() + name.Substring(2);
            }

            return name.Substring(0, 1).ToLower() + name.Substring(1);
        }
    }

this is included in specific converters like

image

and it should part of the generic service when deserialising as per

image

As all this project is WIP ill check in now so you can have a look.

Failing areas are:

as an example AdditionalData in Service and Context in a DidDocument

Edit @veikkoeeva are you happy to look a those above?

veikkoeeva commented 3 years ago

@juanfranblanco I need to be elsewhere the whole day next (day job etc.), but the policy already exist. Example of its use: https://stackoverflow.com/a/58476761.

juanfranblanco commented 3 years ago

@veikkoeeva but that is support "@"? also that is not a biggy can be removed and replace, also make the code better anyway.. what it will require is to be included on custom convertors and on the future generic service. All wip.. we make everything work and then refactor.

juanfranblanco commented 3 years ago

@veikkoeeva I am starting to think that the simplest way to support different json frameworks is to provide a common interface in Common, the each model implementation can provide its own attributes etc.

This way end users which are the ones that will be extending the dids with documents etc (think ceramic) can extend with their framework of choice, and if they want to support both they can do it at their own will, leveraging the best features of both.

For example ExtendedData, Subtypes etc can be implemented leveraging each framework and a end user can use them or extend them in their projects without a worry.

What do you think?

Now as a devil advocate.. you may think if things are going to get extended with a good convertor pattern end users will be able to leverage it better.. But is this what they want to focus their time?

juanfranblanco commented 3 years ago

Futher thought on that.. the STJ implementation will just fit in as there won't be much work todo, now.. the issue will move to the newtonsonf implementation with the Context and the LD, i guess other stuff can fit in pretty easily

Mainly we can have an ServiceInterface that provides the writing and decoding using again IDidDocument as an input etc. Truly will remove the need of any fights with the frameworks, and eventually let nature decide which is best :) And we can concentrate on the crypto which is truly more fun than this. (<-- Defeated, too many hours.. )

veikkoeeva commented 3 years ago

Is there a case where STJ is not available for JSON operations and Newtonsoft is? Are there reasons to prefer Newtonsoft over STJ? As noted earlier, historical download numbers do not (bolded since I edited this later) seem to provide a benefit in this codebase, maybe there are other reasons.

In passing: There probably other ways to have DTOs that can be (de)serialized with these frameworks than interfaces if that's what you propose.

Before any of this planning I would like to get back to a solution that compiles and tests pass using STJ. I think if there is a need to support Newtonsoft we have a number of options for refactoring. Maybe just removing the attributes which I had been planning to take off in any case.

juanfranblanco commented 3 years ago

Yes I have been trying to make the test pass after removing the attributes, (STJ) pain point there is the extending data at the moment and the custom objects. To cheat i tried to inject the attribute JsonDataExtented (stupid idea) and utter waste of time as it will be not in anyway good for a framework.

Overall i am glad the Context works, not in Service the AdditionalData needs to be ignored

juanfranblanco commented 3 years ago

@veikkoeeva One of the most compelling use cases to keep it simple for end users is something like Ceramic https://ceramic.network/, DIDs are an element in both the data structure, schema and of course identity.

Hence the emphasis on compatibility on something which is more widely used and supported that STJ. If you imagine the journey of a user who is storing data in IPFS or anywhere else including DIDs as one of the elements, they might want to introduce these DID or multiple in a big structure that will carry much more information.

The user might have data elements already decorated with attributes from Json.Net, STJ or other, hence my total frustration on incompatibility.

If DIDs were isolated the only issue will be the framework (adding an extra assembly) but they are not. Maybe this is a bigger case to have those interfaces, or maybe not. What do you think?

Edit: As you can see here there is plenty other extensions to DIDs https://github.com/ceramicnetwork some usage of DIDS (https://github.com/ceramicnetwork/js-ceramic/blob/develop/packages/key-did-resolver/src/__tests__/__snapshots__/index.test.ts.snap, and here https://github.com/ceramicnetwork/js-ceramic/blob/develop/packages/3id-did-resolver/src/__tests__/__snapshots__/index.test.ts.snap)

veikkoeeva commented 3 years ago

@juanfranblanco The examples you linked seem to be Jest snap files, not plain DID documents (i.e. they are scaffolded with Jest snap specific structures). As far as I understand by just looking at them, if they were DID JSON documents, I would assume they can be (de)serialized fine.

I try to think of a scenario where user is using .NET Core or .NET5/6 and do not have STJ available. Currently I can think of is that there are DTOs like on those tests in services and the user of the library point-blank refuses to take them as strings and serialize using Newtonsoft and refuses to write a converter and well, just refuses to use STJ and demands Newtonsoft works out-of-the-box.

If there were other cases, I'm still inclined to think that it would be easier to note it's possible later if someone comes along and has a case. Maybe they do a PR (since I think it's doable) and this can be solved then and not now.

Edit: I'm open to hear cases, to be clear. Even if I fail to consider a case now does not such does not exist or I would be vehemetly opposed of solving it right now. I'm more like opposing to put my own cycles to solve it now or to start creating a solution that slows down other work.

juanfranblanco commented 3 years ago

Of course these are just examples, if you look how ceramic works or 3box a Did will be part of the composition. What I mean by composition is that a Did document will part of a bigger schema,

I think this is a better example https://hack.ethglobal.co/showcase/eridanos-recJwuShuxEibN3y7

So yes a user might use STJ or Newtonsoft, but when they have a document including multiple DIDs and verifiable credentials, and extra data (ie tiles using the ceramic example with their own data) they might require all this to be compatible with the framework of choice.

Edit: this is the functional demo https://eridanos.on.fleek.co/

juanfranblanco commented 3 years ago

Regarding scenarios that people may not have STJ, yes i can think of some like blazor, unity, ios etc, also can work in the other way. Hence I am leaning now more towards interfaces to define the schema, as this will allow any user to extend or use the DIDs with their framework of choice without any limitations.

veikkoeeva commented 3 years ago

@juanfranblanco Out of curiosity, do you have examples of such JSON files (without scaffolding)?

So the discussion then would be that someone takes DID DTOs from this library and embeds them into other DTOs of their own and want that Newtonsoft works out of box (*) since they have a want or need to use Newtonsoft instead of STJ? And we are now building a case if we should or should not support such a case right from the start? Would it be sufficient to instruct to usea Newtonsoft converter in that case (I think it should work).

(*) Not just writing a Newtonsoft converter that use this library to (de)serialize the objects but that it needs to work without writing a Newtonsoft converter to do that.

veikkoeeva commented 3 years ago

@juanfranblanco I need to be off from the computer for a while now. So as not to get into a marathon thread where you come up cases and I every time tell that would a converter do, I think one way forwards would be to get a solution that runs the tests.

Then we remove the attributes from the DTOs so that the tests still pass. So then building a solution using Newtonsoft looks could be that someone creates converters and the model does not need interfaces, inheritance or anything to support multiple JSON (de)serialization frameworks. Would that work?

So basically I'm still postponing this upfront work for better design and not pushing it into to this merged spike with the feeling it creates more work and delays. When someone comes along and says that they need this library to work on Unity (I think Blazor has STJ, I don't know about if iOS has something limited there), we know it's possible to do.

juanfranblanco commented 3 years ago

Most of the stuff is framework based, ie mainly all written to ipfs, but as you have seen in the demo you end up composing many objects (i will need to spin up my ceramic stuff. etc) but ceramic works in tiles so the schema is extendable.

So if you are an end user, what will you expect to do? You will not want to create a convertor as that is not the normal scenario, you will want to, start creating your schema, add verification and your other custom stuff, a few dids which may have specific signatures for validation (full doc, not just a resolver link), and finally you might want to serialise all this, create a hash and sign it.

In this scenario you will pick your preferred framework, create your types etc and decorate the attributes and serialize. Obviously you won't do a combination of frameworks as that won't make sense for sure.

Also the user might want to extend the document (DID) to include extra data (typed data) including that information.

In a super ideal scenario we should have been able to use DataMembers and use this as a common attribute for both frameworks, but regardless there is also a limitation as we don't have those JsonDataExtended attributes in system.runtime, hence i was trying to inject the attribute as a hacky way too.

juanfranblanco commented 3 years ago

@veikkoeeva I personally think that it will be the best to have interfaces, is the only solution to share a common model across the two implementation, i personally would use newtonsoft over stj for the time being as it is supported in unity in older versions, blazor and ios (aot).

From a perspective of the STJ implementation will make it much easier as it will allow to move the model classes directly to the STJ. Of course we still need to have converters for the Context.

Edit: Also this way when there is yet another json framework, it can be used across, funny we may have other serialisation methods in the future too. which may rely on other custom attributes in each property.

Most important is that we can keep the contract (interfaces) to be the same.

Edit2: Eventually when all frameworks agree to common attributes, then the model can be moved into Common. Overall is another option to have in place.

veikkoeeva commented 3 years ago

It looks like this to me now:

I think STJ is available in Blazor and Xamarin (iOS) if I recall correctly, maybe not be right now in Unity. STJ is in the framework the recommended way forwards. How many use Unity and need this library right now and do we need to add support for it right now? There was/is a working solution with STJ.

The driving scenario is that that someone has written DTOs and they cannot/do not want to use STJ and do not want to write a converter in Newtonsoft but want/need to use attributes. This is presumably for cases that have started using DIDs and VCs in .NET already now and these scenarios need to be supported. There maybe also something else that allow using STJ without converters if I understand you correctly. I do not know how what's discussed here makes STJ "much easier". I may have misunderstood this scenario.

Then here in this thread of the design space available it is decided DTOs will use interfaces that have attributes, if I understand correctly. We do this design and work upfront right now, or maybe you do and submit the code.

In any case I would like to see an an example of such documents that emdeded DIDs and VCs. I would like to see how DID DTOs with STJ look like and become easier. I'm not very keen to assume maintenance burden without a sample document and a clearer issue than with this thread only, it would greatly ease my thinking to have concrete DTOs that embed these DIDs. I think they would help tests. Even then I might want to consider someting else than interfaces to DTOs if that's the case, at least consider a bit. Even before than that I would like to resume to a working solution and refactor from there, not be in limbo and come up with something that makes some tests pass in the future.

I do not see a proposal that is "easier" so it's really difficult have an opinion on that.

juanfranblanco commented 3 years ago

My problem is that we are not agnostic to a framework, so I am not saying that STJ is going to be great but also I am not saying that Newtonsoft is going to die. It may have its counted days, but it won't be in the near future.

Regarding document examples, i can ask for some.. but truly the most simple thing is that things are going to be composed. STJ vs Newtonsoft both do different things, it is not up to us to discuss this, but to provide a simple way for anyone to do whatever and don't force them in a particular way.

I am creating an example of interfaces, and we can see how bad / good it is. I personally I am liking it as interfaces allow me to extend objects without needing to worry about the overall whole schema. This is similar to your FormatKey implementation using an abstract class.

veikkoeeva commented 3 years ago

All right, we'll see what comes.

This how I understand the scenario: It looks to me you want to explicitly support a scenario where Newtonsoft is used with attributes, not just removing STJ attributes from the DTOs, make the solution compile, tests pass and move forwards and solve the Newtonsoft issue later if a need merges (maybe you have a need somewhere, see following). Not only that, you want to support both STJ and Newtonsot with attributes, also in environment where the users have both available and has a strong preference to use in all cases either STJ or Newtonsoft via attributes, or Newtonsoft in this case since STJ case exist already.

juanfranblanco commented 3 years ago

Yes that's about it :) !!

juanfranblanco commented 3 years ago

@veikkoeeva check the current implementation using interfaces https://github.com/decentralized-identity/did-common-dotnet/tree/spike-interfaces there are some issues on the round trips of full documents and some IService but.. you can see the overall idea. Once the interfaces are sorted it is surely much easier than fixing with no attributes, I have restored the old attributes too.

tmarkovski commented 3 years ago

The only argument I can make for Newtonsoft is the dependencies on other libraries, specifically, those supporting JSON-LD processing. Currently, dotnetrdf and json-ld.net both use Newtonsoft. If the intention of this library is to allow upstream implementation to utilize the data model, while being able to use the JSON-LD processing features, it might be better to support both.

juanfranblanco commented 3 years ago

@veikkoeeva https://github.com/decentralized-identity/did-common-dotnet/tree/spike-interfaces migration to interfaces is done now, tests are passing STJ only no Newtonsoft.. check the structure for thoughts etc. I had to changes to support interfaces but mostly was already handled by the convertors..

juanfranblanco commented 3 years ago

Thanks @tmarkovski yes that is the thought, although it is proving a small headache, check that interface spike if you have any thougths or feedback, the only issue here is the duplication of the models, but there is no much way around it. Ok bed time.. :)

veikkoeeva commented 3 years ago

@tmarkovski I think both Dotnetrdf and json-ld.net support POCOs. This is because they are built on Newtonsoft that supports POCOs at its core case. If this is the case, I believe we have this covered in the future. I think they also support converters. It looks like this was a general observation. So I believe it is an unblocked future scenario.

Referring to other isuses and this thread with @juanfranblanco, if you have a specific scenario in mind that needs to be supported right now, I would like to see a more concrete use case we can also test to see it is covered and remains covered. It can also be just an example of something so that we can now think if it's unblocked even if not acting on it right now.

@juanfranblanco I thought the thought was

[Veikko, referring to other discussion] This how I understand the scenario: It looks to me you want to explicitly support a scenario where Newtonsoft is used with attributes, not just removing STJ attributes from the DTOs, make the solution compile, tests pass ...

[Juan, answering] Yes that's about it :) !!

I think it would be simpler to make them DTOs POCOs. Remove attributes. Then we would have one set of DTOs as POCOs and two sets of converters. DTOs as POCOs without interfaces.

Even using Newtonsoft I could very well imagine that if you have a case where a parent element or an element preceding in element order is telling the next type to read, as DID specifies, a converter would be written. Doing this with attributes is not a straightforward case.

Assuming someone just refuses to do this and insists on attributes, one another way without interfaces could be

public class BasePoco
{
    public string SomeField { get; set; }
}

public class NewtonsoftPoco: BasePoco
{
    [SomeNewtonsoftAttribute]
    new public string SomeField { get; set; }
}

(I just wrote that on top of my head, also maybe generators can be used, and other ways)

which would still allow attributes. In this case, like in this currently being built, we could reasonably ask if it would be a case for this library to support and distribute or could we reasonably instruct those potential users who insist using Newtonsoft attributes only to do this themselves. I can also think that if we want to maintain this as a core case and we insist on not making use of converters but attributes only, it can be that beyond just adding interfaces and two sets of POCOs, we need to start alterting the types and processing in other ways. If it were become apparent we would need to process anything in very certain ways to build a case for attributes only Newtonsoft, then I feel the scenario needs to be balanced with others.

I think using converters here is reasonable. I think we can reasonably expect users write converters as part of their other developer flows where they embed POCOs from this library and so adding a set of already written converters from this library is not an issue. When they do not embed DTOs from this library but use this libary as-is, they maybe do not need to see converters being used.

I of course would like to build a scenario why would we now insist not using DTO POCO by way of refactoring out the STJ attributes? Does it block us from some use cases we can think of? I do not think it blocks cases where one uses Newtonsoft or libraries based on those. Using POCOs likely warrants using converters by way of how DID specification is written, I think.

Moreover in general, why would we not write a more thoughtful issue about this? Why cartwheel this through in a thread where goal posts are moving and I try to dig up something (see the quotes)? Why not write an issue that entertains the scenario and some alternative designs? Why insist it goes into a spikes folder and not be a PR towards a working base like I could imagine this could be done? Do we in the future say we support every way of implementing things by way of it's not up to us pick supported scenarios?

This way of working feels a lot of work to me, maybe we can be more organized (it may be my preference only). It is compounded by that I do not have any more time to put into this this week and maybe very little next week. So one way of moving towards is that I just note it's all fine to me and I come back in a week or two to see what has happened with all the code moving around and use case scenarios implemented. I also don't want to stand in a way if things are moving.

veikkoeeva commented 3 years ago

@juanfranblanco If all you was add interfaces and the code compiles, I think the tests should pass and things work. See that of some notes on what could be an alternative design. It depends on how pathological the scenario is, of course.

juanfranblanco commented 3 years ago

@veikkoeeva Mainly this is what I have been doing, finding the best solution that allows any framework to work, have the lesser impact in your previous work but also maintaining for an end user all the capabilities of their chosen framework. Also I know that your busy so I want to put the less stress on you by providing you constant feedback on what is happening. But this is a great experience to get a good understanding of how each other see this project going in the future and obviously the pros and cons.

I am trying to get your best feedback as my ideas might be completely wrong as I am trying to experiment the different alternatives, without imposing any. Also I want to get the architecture out of the way so if we are happy with the overall structure we can convincedly say why everything is setup this way, currently and what i may look like in some years.

So after structuring the code. In one side I have been trying to eliminate the need of using attributes or use common attributes which provides a common model for both (ie not interfaces), similar to your current thoughts, and as we have seen the other alternative is to use interfaces. In both areas I tried make working solutions as fast as I can to validate pros and cons.

As you have seen we have found plenty issues, mainly STJ does not support yet a common set of attributes (DataMember, DataContract) sadly as we have seen per the issues that decided against that due to the lack of features they have implemented, mainly not able to provide ordering etc etc. We know that possible this is coming after .Net 6 so in a couple of years time, this can be achieved.

This was pretty sad, and truly if it was a project that is not going to be extended by an end user I would not mind (mainly use POCOs) and come up with a complex way to handle AdditionalData. Talking of AdditionalData NewtonSoft handling of additional and attribute is all done internally in private classes https://github.com/JamesNK/Newtonsoft.Json/blob/666d9760719e5ec5b2a50046f7dbd6a1267c01c6/Src/Newtonsoft.Json/Serialization/DefaultContractResolver.cs#L443-L579, yet again this is pretty sad as an end user may want to change how AdditionalData is used as they extend the classes, etc using attributes. So mainly I inspected all the code and both frameworks and there was not an error prone way to enable end users to use an alternative. Both recommend the use is to use their attributes.

In a a truly ideal scenario, and something to push to the OSS community, both and any framework would share a common set of contracts / attributes etc as we had seen in the past what happened with Dependency Injection. Of course this not limited to this, but I was hoping to provide the users with the best experience they need, in a similar way that Javascript users when they deal with Json for them it just works.

So I pondered onto the other solution which was to have a common set of interfaces, I personally was not keen as I wanted to have a common model for both, this is your POCO, but so far seems to be the more flexible way to define the contract of how the model should work and leaving each framework, or future framework as we might see other ways to serialize and deserialise the data.

Obviously having interfaces brings their own set of issues, mainly the model needs to be duplicated in the implementation project, but this way any end user can use their preferred framework and extended the best way they like it.

Another solution could be to simplify the models and convertors, if this is even possible, so any end can easily modify or extend the convertors, simplifying inheritance selection of verification relationships, formats, the context etc. But I truly have not wanted to disrupt your initial code as you have put plenty thought on it (it can be seen by working on it :) ). In any case as things develop we will see the areas that might need to be simplified or extend more.

Regarding the way of working I know that you have been busy, hence I created the two solutions highlighting the issues, one in the current main area, and the other in https://github.com/decentralized-identity/did-common-dotnet/tree/spike-interfaces. All the tests pass in the interfaces, whilst this was as well painful the migration, as I needed to pin point all the different places of activation / creation of classes, inheritance logic, etc it was much easier than fighting with the frameworks by removing their features (thinking as an end user) Hence I am leaning now towards more to the interface solution.

Once we pass this hurdle, hence I wanted to get it out of the way, things will be ideally super simple as we will be able to concentrate in specific features, and do the usual stuff. This is were we are with lots of ideas and merging them into a common architecture :) So please have a look at both areas, it takes truly 5 minutes to see what is happening in each side and we can take it from there. I personally going to start to create the deserialisation of Newtonsoft now that STJ is working so we can take it from there in this common model.

Overall my principles are, making it easy for a user to work, extend and don't break any solution / framework or way of working and seeing how DIDs are being used across different languages (which are years light ahead of .Net) how this can help to achieving at least something that is the same and compatible with them.

Truly this is pretty hard I know, and gives you an understanding of why MS (as an example with some Azure projects) decides in some areas to keep things whilst open source in their product projects, as it is much easier to focus in a product than to please everyone with a framework.

juanfranblanco commented 3 years ago

Now the POCO solution using inheritance, is actually taking us to the interfaces

public class BasePoco
    {
        public string SomeField { get; set; }
    }

    public class NewtonsoftPoco: BasePoco
    {
        [SomeNewtonsoftAttribute]
        new public string SomeField { get; set; }
    }

so if you look at here:

https://github.com/decentralized-identity/did-common-dotnet/blob/spike-interfaces/src/DidNet.Common/IService.cs

using System;

namespace DidNet.Common
{
    public interface IService: IAdditionalData
    {
        Uri? Id { get; set; }
        string? Type { get; set; }
        string? ServiceEndpoint { get; set; }
    }
}

if ends up in STJ like this.. https://github.com/decentralized-identity/did-common-dotnet/blob/spike-interfaces/src/DidNet.Json.SystemText/Service.cs#L10-L29

/// <summary>
    /// https://www.w3.org/TR/did-core/#service-endpoints
    /// </summary>
    [DebuggerDisplay("Service(Id = {Id})")]
    public class Service : IService
    {
        [JsonPropertyName("id")]
        public Uri? Id { get; set; }

        [JsonPropertyName("type")]
        public string? Type { get; set; }

        [JsonPropertyName("serviceEndpoint")]
        public string? ServiceEndpoint { get; set; }

        [JsonExtensionData]
        public IDictionary<string, object>? AdditionalData { get; set; }
    }

leaving the original model intact and not having to think of using in STJ the yet to be supported DataMember

Now we need to discuss what the namespaces will look like if we go with this approach.

Now.. as everything is an interface everything can be easily extended. So a user may decided to have their own service like as seen in the test DTOs without any impact on the attribute usage of [JsonExtensionData]

  [DebuggerDisplay("SocialWebInboxService(Id = {Id})")]
    public class SocialWebInboxService: Service
    {
        [DataMember(Name = "description")]
        public string? Description { get; set; }

        [DataMember(Name = "spamCost")]
        public SpamCost? SpamCost { get; set; }
    }

of course ExtensionData can be identified by convention as seen here

    public interface IAdditionalData
    {
        IDictionary<string, object>? AdditionalData { get; set; }
    }

now talking about Interfaces what will happen if we wanted to create a common contract for SocialWebInboxService in the common we could create the ISocialWebInboxService,

    public interface ISocialWebInboxService: IService
    {
        string? Description { get; set; }
        ISpamCost? SpamCost { get; set; }
    }

what are the benefits?

The implementation can inherit the contract IService and ISocialWebInboxService, but at the same time inherit the ServiceSTJ.

  [DebuggerDisplay("SocialWebInboxService(Id = {Id})")]
  //assumed in their own project so no need to mark as (STJ suffix)
    public class SocialWebInboxService: Service, ISocialWebInboxService
    {
        [JsonPropertyName("description")]
        public string? Description { get; set; }

        [JsonPropertyName("spamCost")]
        public ISpamCost? SpamCost { get; set; }
    }

Now the NewtonSoft implementation, knows that they need to implement too the ISocialWebInboxService and add it to its resolver list the way the want to.

The way I see it is that the models (POCOs) will be very specific to your type of framework although ideally they wont DataMembers etc in the future.

A common ground could be achieved too, having both interfaces and POCOs with DataMember and DataContract attributes, but those can be extended locally to include the Extended Data attribute.

Here sadly if you need to add extra attributes you will need to create new properties

/// <summary>
    /// https://www.w3.org/TR/did-core/#service-endpoints
    /// </summary>
    [DebuggerDisplay("Service(Id = {Id})")]
    public class ServiceSTJ : Service
    {
        [JsonPropertyName("id")]
        public new Uri? Id { get; set; }

        [JsonPropertyName("type")]
        public new string? Type { get; set; }

        [JsonPropertyName("serviceEndpoint")]
        public new string? ServiceEndpoint { get; set; }

        [JsonExtensionData]
        public new IDictionary<string, object>? AdditionalData { get; set; }
    }

Edit: @veikkoeeva ^^^

veikkoeeva commented 3 years ago

So after structuring the code. In one side I have been trying to eliminate the need of using attributes or use common attributes which provides a common model for both (ie not interfaces), similar to your current thoughts, and as we have seen the other alternative is to use interfaces. In both areas I tried make working solutions as fast as I can to validate pros and cons.

STJ, Newtonsoft, some CBOR (e.g. System.Formats.Cbor) or whatever random (de)serializer one chooses support POCOs without attributes if that is the goal. This is how I think currently. In addition to this, you seem to have a goal to support attributes only Newtonsoft type hierarchy in addition to POCOs, if understand correctly. I think this case can be realized attributes only Newtonsoft types inheriting from POCOs. STJ cannot be made to work without converters in any complex scenario, it is how it has been designed. I keep repeating this, I see.

POCOs can be made by removing the attributes that currently do:

Do we not know how to remove these attributes? I was thinking to do that but the code was merged (and reorganized) before that.

Do we want to avoid converters at any cost with Newtonsoft support? Why? Do we intend to uphold this decision in the future?

If we support attributes only Newtonsoft processing without converters in the future, is it necessary this decision affects all other types and processing also? See the following where I go back to the earlier point I keep repeating.

STJ certainly cannot be made to function without converters in any sceneario that is more complicated, it is how it has been designed. DID specification, and later VCs, presentation exchange and so on seem to be such complex scenarios. STJ can support POCOs without attributes, so can Newtonsoft as far as I know.

Now the POCO solution using inheritance, is actually taking us to the interfaces

While I did not think it much, it looks to me that particular solution would not force interfaces to all type hierarchies. In this case attributes only Newtonsoft that will not use converters will inherit from POCOs. It looks simpler to me if the intention is support this particular scenario, I'm inclided to think metrics would support this. Does it work?

By inheriting the attribute only Newtonsoft that will not use converters types from POCOs seem to be another alternative that is less intrusive. Does it work? If it does, can that be used? If not, do we need to proceed with this idea and with what looks to me a self-imposed constraint of attributes only Newtonsoft without converters.

Do I misunderstand something?

Is is not only that, other future processing that need to happen via interfaces and not via data types as far as these parts are concerned. It looks to me this decision of attributes only Newtonsoft without converters constructed using interfaces and two type hierarchies with attributes has implication to whatever other processing and design there will be.

If we take it upon ourselves to impose a constraint that all processing in the future must be possible in Newtonsoft using attributes only since we do not want to use converters, it is intrusive on other parts of design. Is this scenario so important? I think this not only for this case. Since maybe this one can be solved if attributes only Newtosoft without using converters by inheriting from POCOs that others can use without interfaces and other potential constraints.

This future processing I see is VCs and presentation exchanges where I think STJ also needs converters and I had thought about going with about the same solution. But it might be more complex because of attributes only Newtonsoft that will not use converters implemented by interfaces and two different type interfaces that is on the design here.

I believe the mentioned upstream libraries that use Newtonsoft in their core allow using converters. Is this not the case? Even if that is not the case, see the previous of making DIDs, VCs and presentation exchange to use attributes only class hierarchies without converters.

Once we pass this hurdle, hence I wanted to get it out of the way, things will be ideally super simple as we will be able to concentrate in specific features, and do the usual stuff.

It certainly looks like you want to make this change and others. I keep repeating that why not POCOs if it support the case you wrote and I quoted. I keep repeating it because you explain to me these type hierarchies are needed to support STJ and Newtonsoft. It looks like wanting to support attributes only Newtonsoft specifically – so that converters would not ever be needed. I am not sure if that can remain the case in the future. The tradeoffs you propose over POCOs to support attributes only Newtonsoft without converters do not seem appealing to me if we can supports attributes only Newtonsoft without converters so that attributes only Newtonsoft without converters types inherit from POCOs.

juanfranblanco commented 3 years ago

Thanks @veikkoeeva we are truly in the same page. We just need to validate the best approach for end users.

STJ, Newtonsoft, some CBOR (e.g. System.Formats.Cbor) or whatever random (de)serializer one chooses support POCOs without attributes if that is the goal.

Exactly hence an interface might fit better to match the structure

public interface IService: IAdditionalData
    {
        Uri? Id { get; set; }
        string? Type { get; set; }
        string? ServiceEndpoint { get; set; }
    }

Although having the capability to add DataMembers to describe the real name in the contracts is pretty important, as we know interop with other systems is necessary so it is easy to understand the json etc structure.

I removed the attritutes on the main branch, and replaced them with DataMember attributes, also here is where the limitations surfaced (thinking as an end user and end consumer)

BTW I am not saying that convertors are not going to be needed, I am actually now creating a convertor now in Newtonsoft for the VeficationMethod trying to be similar to the STJ implementation, so it is easier to maintain (and of of course as the structure truly dictates so) but what I want is for users to be able to pick and chose without any constraints if they want to use attributes and extend as they wish.

I think for an end user having examples using attributes so they can understand their inner and outs and can extend might be key, but it might be that is the opposite in some scenarios, or impossible due to inheritance / strategies, etc.

In the previous post I am highlighting the different pros and cons of having either POCOs, interfaces and a combination of both. We can continue to add other examples to see what will be best, i am personally working on the interface branch as all the tests are passing there.

Just to reinforce I am in no way committed to anything, but my principle is that users should not need to suffer if it is too complex they will end up using Javascript instead.

veikkoeeva commented 3 years ago

@juanfranblanco Why not having a plain POCO Service? What benefit does interface bring? Now it needs to be implemeted by a POCO. STJ does not need attributes, I don't know what will. DataMebers can be added to POCOs too without introducing additional depedency (since they're in base class libaries may affect binary size, AOT etc.), albeit I don't know if it's useful.

All examples can be realized by inheriting from POCOs with attributes added?

veikkoeeva commented 3 years ago

I feel sorry also I need to leave this discussion like this for a while. It may be I can get back next week. I'm mostly on the move and need to accomplish other work also.

juanfranblanco commented 3 years ago

Sure don't worry I understand :) regarding the POCO vs interfaces this is what I was highlighting in the previous post, I think we can take this offline when you have more time. But overall interfaces can dictate the contract structure and mapping without forcing you to a given inheritance, attribute usage etc that the POCO dictates. And yes I truly hate having to copy the model into another implementation, hence my frustration as it most scenarios this might have been solved by having these common attributes.

You actually have done something similar to an interface, by having the Format abstract class with no internal implementation. Like I said in the other post another alternative might be to have iboth interfaces and POCOs model in Common, so this way we can achieve the best common ground, allowing both for extensibility and or fully replacement if wanted by implementing the interface.

juanfranblanco commented 3 years ago

@veikkoeeva when you have a second check the https://github.com/decentralized-identity/did-common-dotnet/tree/spike-interfaces/src/DidNet.Json.Newtonsoft/Converters and overall port to Newtonsoft, I have tried to keep it the same as possible as the STJ so it is easier to maintain (and obviously create ;) ).

I think this will be great point of reference as we can see the pain points but also the duplication as we see with the POCOs and hopefully achieve something that can work for all. If there is any consolation I have just seen that JSON LD are also going to port and support both frameworks https://github.com/linked-data-dotnet/json-ld.net/issues/59#issuecomment-653392194 in the future, so mainly everyone is in the same boat.

BTW lots of great thoughts in the STJ code base, great to work with that.

juanfranblanco commented 3 years ago

@veikkoeeva check the latest https://github.com/decentralized-identity/did-common-dotnet/tree/spike-interfaces/src/DidNet.Json.Newtonsoft/Converters when you have a second, no pressure, all the convertors have been ported. I am going to try to move the model if both scenarios (STJ and Newtonsoft back to common but keeping the interfaces to enable more extensibility, then simplify abstract the convertors creation in newtonsoft as there is no factories.. hopefully something that can be extendable) Lets see how that looks and can be applied to STJ or try something different.

juanfranblanco commented 3 years ago

@veikkoeeva Further update, if you check the latest commit https://github.com/decentralized-identity/did-common-dotnet/commit/1cebb6f087c95439d2e347816b12e584b0d85515. This combines back the models, keeps the interfaces and introduces extending models. All the models are decorated with DataMember and DataContract attributes where necessary.

Overall these are the findings, which provide good justification of structure.

    [DebuggerDisplay("Service(Id = {Id})")]
    [DataContract]
    public class ServiceExt : Service
    {
        [JsonExtensionData]
        public override IDictionary<string, object>? AdditionalData { get; set; }
    }

This way a user remains full in control of the objects without adding dependencies to the model.

Example of issue:

 "@context": "https://www.w3.org/ns/did/v1",
    "authentication": [
        "did:example:123456789abcdefghi#keys-1",
        "did:example:123456789abcdefghi#keys-3",
        {
            "controller": "did:example:123456789abcdefghi",
            "id": "did:example:123456789abcdefghi#keys-2",
            "publicKeyBase58": "H3C2AVvLMv6gmMNam3uVAjZpfkcJCwDwnZn6z3wXmqPV",
            "type": "Ed25519VerificationKey2018"
        }
    ],
    "context": "https://www.w3.org/ns/did/v1",

Next it will be to experiment with a simpler way to instantiate and configure the opptions, convertors etc. From there anything is up to discussion, but we have a common ground to support both frameworks and to demonstrate benefits / issues from each framework. Nevertheless we can see how STJ is heavily based on Newtonsoft anyway. But as with .Net and .Netcore etc and don't see this going away in many many years.

veikkoeeva commented 3 years ago

Sure don't worry I understand :) regarding the POCO vs interfaces this is what I was highlighting in the previous post, I think we can take this offline when you have more time.

Online is fine.

Further update, if you check the latest commit 1cebb6f.

I'm not sure what to look at. Instead I'll put some effort on the (massive) PRs that are open.

Overall these are the findings, which provide good justification of structure.

Obviously duplicating the whole model makes no sense in different implementations

All extension objects are suffixed with "Ext" and in their own namespace as per @veikkoeeva suggestion.

All the model objects properties are virtual so they can be overriden to extend with attributes. For example includes NullValueHandling attributes as these were not ignored by the serializer

I think null values were handled with converters in STJ already and could be with Newtonsoft too. I don't remember proposing suffixes. I agree duplication is not needed, POCOs suffice on the core.

More broadly:

If the concern was to get Newtonsoft onboard, I think it could have been done by adding Newtonsoft converters and tests that excercise them (that too could have been a separate PR). Maybe add cases where specifically Newtonsoft could be used instead of STJ, e.g. with some particular library and see the code functionality enhanced with Newtonsoft (converters, and attribute-added types?) realize the case and as a way of example. There is still the case if STJ should be decoupled from core, it can be done easily I believe, but when doing that maybe remove other unneeded "standard references".

To reflect specifically to the topics here, soon after the work-in-progress merge of the branch and consequent reorganization and while this discussion was running, I updated the version with the changes I had mostly done to remove STJ attributes, added more tests and fixed some tests.

Now this codebase is quite different and I think here the idea is to do something different even if broadly the same approach is used. Which leads me to following point:

I do not have capacity to work like on spikes directory to pursue 'correct architecture' and build support on everything upfront.

I have a functional codebase (currently on my personal and company account). At the moment it is more important for my other goals to have a working version I can improve. If this codebase here is broadly based on same basic ideas, there may be possibility for cross-breeding code and I can try to keep up and help here if that is desirable.

Certainly I had already a plan to be able to make this useable with other ideas, such as DIDs (and VCs) inherting from Newtonsoft or STJ types by way of making them "hierarchical key-value bags" or even supporting a case of JSON to-the-whitespace that is signed (which likely means one should look into some proper signing scheme). I also try to look this so it is useable in more regulated environments, like payments systems.

Currently I have small refactorings in mind, in the order of adding Newtonsoft converters-only (or maybe CBOR), to refactor types and cryptograhic parts and tests. While I note this, I add that likely I work first on VCs and presentation exchange and so maybe there's a chance to cross-breed ideas here in the future.

juanfranblanco commented 3 years ago

@veikkoeeva I kept it as much as possible to match your code grab the pr and have a look, so you don't have to worry and can be built upon there. There are Newtonsoft convertors etc. I can give you an overview offline, it is not as different as it may sound in the first place. Check the latest or, it can be merged and work from there. I understand having proprietary code etc, so good to be aligned.

juanfranblanco commented 3 years ago

@veikkoeeva i created this pull request, you will see that not much have changed but now it enables the architecture to include newtonsoft, but most important now that this is enabled we can validate each one of the models. https://github.com/decentralized-identity/did-common-dotnet/pull/10

veikkoeeva commented 3 years ago

@juanfranblanco I added a review. I apologize for the delay.