TrackableEntities / EntityFrameworkCore.Scaffolding.Handlebars

Scaffold EF Core models using Handlebars templates.
MIT License
209 stars 53 forks source link

[Guidance] How to properly deserialize entity graphs coming from the server? #64

Closed omatrot closed 4 years ago

omatrot commented 5 years ago

I see two potential problems: 1/ Entities on the server are pascal cased on the server while camel cased on the client. 2/ What about object references looping and duplicates?

I suppose the other way around there is something to do too.

tonysneed commented 5 years ago

Hi @omatrot, welcome!

Let's move these issues over to EntityFrameworkCore.Scaffolding.Handlebars, which is where the code gen takes place.

To respond to your items:

1/ Entities on the server are pascal cased on the server while camel cased on the client.

This is the default behavior of ASP.NET Core, so there should not be a problem. Have you tried it yet?

2/ What about object references looping and duplicates?

Good question. In .NET there is a setting which handles cycles in the graph. On the client, you best bet may the the Dcerialize npm package, which is compatible with Json.Net on the server.

omatrot commented 5 years ago

Hi Tony, I'm @omatrot on Twiter :) I'm using trackable-entities-js. 1/ I'm sending Data through SignalR and found a way to use camel casing properties like this: return JsonConvert.SerializeObject(wholeResult, new JsonSerializerSettings { ContractResolver = new CamelCasePropertyNamesContractResolver(), ReferenceLoopHandling = ReferenceLoopHandling.Serialize, PreserveReferencesHandling = PreserveReferencesHandling.Objects }); 2/ I had no clue about Dcerialize, will check it out.

Thanks for the amazing work.

omatrot commented 5 years ago

Dcerialize works only with annoted properties.

//unannotated properties are not serialized or deserialized, they are totally ignored That would be problematic right now.

I assume that entities constructors must be called for the whole tracking to work properly ?

tonysneed commented 5 years ago

Dcerialize works only with annoted properties.

Can you update the hbs templates to include the annotations?

I assume that entities constructors must be called for the whole tracking to work properly ?

Yes, you need the ctor to return super.proxify(this).

omatrot commented 5 years ago

Can you update the hbs templates to include the annotations?

I'm afraid I'll need to do that, will start by manually modifying a few entities though.

Yes, you need the ctor to return super.proxify(this).

I may be wrong, but standard JSON deserialization will not call constructors, so Dcerialize should be used as it will call them.

tonysneed commented 5 years ago

I may be wrong, but standard JSON deserialization will not call constructors, so Dcerialize should be used as it will call them.

The ctor does not need to be called during serialization. Rather it's called in the client (SPA) app to set the ITrackable properties (trackingState, modifiedProperties), which are themselves serialized.

omatrot commented 5 years ago

I'm talking about deserialization on the client indeed.

tonysneed commented 5 years ago

You should be fine then. Have you tried and run into problems?

omatrot commented 5 years ago

Right now I'm using standard JSON deserialization on the client (Javascript). Change tracking is not working, despite setting tracking to true, because of the missing call to the entity constructor during deserialization.

I guess I need to use Dcerialize for the constructors to be called and change tracking to work properly

tonysneed commented 5 years ago

Ah yes, I believe Dcerialize will help then.

Otherwise you would need to call the Proxify method on deserialized entities.

omatrot commented 5 years ago

Calling the proxify method on deserialized entities fails with Class constructor TrackablEntity cannot be invoked without 'new' So I guess I'm stuck unless I have a proxify method that use Object.create() instead?

tonysneed commented 5 years ago

Can you paste the code here?

omatrot commented 5 years ago
// this.datiEntityToEdit is a deserialized entity
const obj = ObservableEntity.proxify(
      Object.getPrototypeOf(this.datiEntityToEdit).constructor
    );
tonysneed commented 5 years ago

Does the entity have a constructor?

omatrot commented 5 years ago

Yes:

  constructor() {
    super();
    //return super.proxify(this);
  }
tonysneed commented 5 years ago

Does web pack remove the constructor? What happens if you add console.log?

tonysneed commented 5 years ago

Perhaps you could reproduce the issue with a small GitHub repo?

omatrot commented 5 years ago

The constructor is called, This is the call to super(); that throws the error.

But I think this is my fault because I have es5 instead of es2015 in .tsconfig target compiler option. Setting 'es2015' actually leads to errors because of circular dependencies between my entities.

Didn't you had this problem with your sample ?

tonysneed commented 5 years ago

You can’t use the es5 compilation option because proxies are an es2015 feature that cannot be transpiled to es5.

My sample does not perform deserialization. But I can modify it to do so.

Doesn’t dcerialize solve circular references?

omatrot commented 5 years ago

Yes it does, but here I'm talking about circular references from the angular point of view:

WARNING in Circular dependency detected:
src\app\domain\models\Device.ts -> src\app\domain\models\Registration.ts -> src\app\domain\models\Device.ts

WARNING in Circular dependency detected:
src\app\domain\models\Fleet.ts -> src\app\domain\models\FleetDetail.ts -> src\app\domain\models\Fleet.ts

WARNING in Circular dependency detected:
src\app\domain\models\FleetDetail.ts -> src\app\domain\models\Fleet.ts -> src\app\domain\models\FleetDetail.ts

WARNING in Circular dependency detected:
src\app\domain\models\GpsBoxConfiguration.js -> src\app\domain\models\VehiculeGpsBoxInfo.ts -> src\app\domain\models\GpsBoxConfiguration.js

WARNING in Circular dependency detected:
src\app\domain\models\Registration.ts -> src\app\domain\models\Device.ts -> src\app\domain\models\Registration.ts

WARNING in Circular dependency detected:
src\app\domain\models\Vehicule.js -> src\app\domain\models\VehiculeGpsBoxInfo.ts -> src\app\domain\models\Vehicule.js

WARNING in Circular dependency detected:
src\app\domain\models\Vehicule.ts -> src\app\domain\models\VehiculeGpsBoxInfo.ts -> src\app\domain\models\Vehicule.js -> src\app\domain\models\FleetDetail.ts -> src\app\domain\models\Vehicule.ts

WARNING in Circular dependency detected:
src\app\domain\models\VehiculeGpsBoxInfo.ts -> src\app\domain\models\Vehicule.js -> src\app\domain\models\VehiculeGpsBoxInfo.ts

EDIT:

My angular module is importing several entities, and I think that this is the source of the problem:

import { Vehicule } from "../../../../../domain/models/Vehicule";
import { GpsBoxConfiguration } from "../../../../../domain/models/GpsBoxConfiguration";
import { GpsBoxType } from "../../../../../domain/models/GpsBoxType";
tonysneed commented 5 years ago

Two ways to deal with the Angular warning.

  1. Set "showCircularDependencies": false in your .angular-cli.json file.
  2. Put all the entities into a single .ts file. You should be able to do this in .tsconfig.app.json.
omatrot commented 5 years ago

I've pulled all the entities in one single ts file and it solved the warning during compilation. I'm also back to using ES6 target.

This is going to sound crazy, but everyting is fine until I simply add code using Dcerialize (in my case DeserializeArray). With this code that is not even called yet, the angular module complains that one of my entity cannot be used before being instantiated.

If I'm using ES5 target, the angular module is OK.

EDIT: This seems to be a bug in Angular.

tonysneed commented 5 years ago

Sounds like your were able to get past the circular dependencies issue by specifying a single .ts output file. This is the same issue as the Angular bug you mentioned.

The other issue seems like it is specific to Dcerialize, so maybe open an issue there? It seems to have to do with deserializing entities with constructors.

omatrot commented 5 years ago

Unfortunately, issues are not enabled in the Dcerialize repo :(

tonysneed commented 5 years ago

Maybe check these out? https://stackoverflow.com/a/22245105

omatrot commented 5 years ago

I may be wrong but all are creating simple JSON objects with only properties. Maybe this one could do if it understands JSON.NET references

omatrot commented 5 years ago

Ok I've been able to reproduce the problem on your trackable entities sample simply by adding Typescript decorators on entities.

I've ben using serializr to successfully deserialize entities. I just need to handle JSON.NET refs by myself and deal with arrays that are not supported right now. It works because there is an API to mimic the decorators to tell how (de)serialization should go.

I'll report here as soon as I have a working example with my entities.

tonysneed commented 5 years ago

serialzr looks promising. Regarding cycles, you might want to consider creating separate entities for serialization that don’t have cyclical references, using something like AutoMapper (see https://medium.com/ps-its-huuti/how-to-get-started-with-automapper-and-asp-net-core-2-ecac60ef523f). It sounds like Json.Net’s ref format isn’t interoperable.

omatrot commented 5 years ago

serializr has its own ref format that is not suitable to my needs. I'm afraid I'm stuck, unless I have a way to turn dcerializer decorators into something that works with angular (like the corresponding API that serializ provides).

tonysneed commented 5 years ago

Closing this for now, since best practice is to avoid sending entities over the wire that have cycles and use auto-mapper to handle the data transfer.

omatrot commented 5 years ago

Just to be clear, this as nothing to do with circular dependencies. It's more about types being used before being declared. I forked dcerialize to try to provide an API to mimic the decorators, as serializr does.

tonysneed commented 5 years ago

Oh yes, was thinking of another issue. 😄 You mentioned JSON refs, but those would go away without JSON.NET preserving references to handle cycles.

I imagine using POJO's (Plain Old JavaScript Objects) as DTO's (Data Transfer Objects) would omit a ctor and solve your serialization issue?

omatrot commented 5 years ago

Why not try it with your tackable entities javascript sample augmented with a Web API sending the initial data and receiving the modified version ?

tonysneed commented 5 years ago

Good idea to update the sample with an API. Let me see if I can find some time to do that.

omatrot commented 4 years ago

Hi @tonysneed. Did you had any time to look at it ?

tonysneed commented 4 years ago

Haven’t had time for this. @lelong37 Does the URF sample use trackable entities?

omatrot commented 4 years ago

Hi @tonysneed. I have been in touch with the creator of dcerialize (which is a french guy like me). He told me to set emitDecoratorMetadata to false and see how it goes. I tried in a fork of your trackable entities js sample, and it seems to work.

I'll try that in my project and get back to you with results.

tonysneed commented 4 years ago

That sounds like great news! If all goes well, why don’t you submit a pull request?

omatrot commented 4 years ago

This works because decorator metata are mandatory for dependency injection in angular <= 7.5. They are not anymore starting with Angular 8. I guess I'll close the issue, because I can upgrade to Angular 8.