emfjson / emfjson-jackson

JSON Binding for Eclipse Modeling Framework
https://emfjson.github.io
Other
80 stars 23 forks source link

Support for containment proxies #78

Closed ghillairet closed 8 years ago

ghillairet commented 8 years ago

Make sure containment proxies are supported, see related #76.

Dufgui commented 8 years ago

Hello,

I need this feature but I am not very fluent with EMF. So could i help you on this ?

I create a small unit test. But i don't know where i must start. I think but i am not sure:

In Xml, even if the ref is not in the model, they get it an create a proxy object. I think you already know the problem, because in containmentTest with the file test-proxy-5b.json . The real load not work (we just have an object but without the proxy url and not loaded)

PS : Do you speak french ? because we have the same first name, and so i am ;o) ?

ghillairet commented 8 years ago

Salut, yes I'm French ;)

The problem is during loading, I believe serialization works for containment proxies. The test that fails during loading is this one https://github.com/emfjson/emfjson-jackson/blob/containment-proxies/src/test/java/org/emfjson/jackson/junit/tests/ContainmentTest.java#L194

I have to look at it in more details to see what's wrong.

Dufgui commented 8 years ago

Hello,

yes i confirm, the serialization work. And you point the exact unit test that i think. I think add an assertion on label of child could be a first step.

ghillairet commented 8 years ago

I have a fix for it in branch containment-proxies

But for now it only works if your containment reference has it's attribute resolveProxies set to true.

Dufgui commented 8 years ago

i will test if my case work on tomorrow (i al off today and don"t have access to the code). Thanks a lot. If not, and see a problem, i try a PR.

thanks again

Dufgui commented 8 years ago

Before testing, i make small review of your code. Just a small point: I think you miss:

if (!value.eIsProxy()) { entries.store(resource, value);

for object not in array line 290 of EObjectDeserializer, no ? because object must have the same treatment if they are in array or in a direct reference no ?

ghillairet commented 8 years ago

Yes you are right, that's missing.

Dufgui commented 8 years ago

PR done, and i add type for external proxy as emf xmi do.

ghillairet commented 8 years ago

I pushed changes for this in master, if you can try and tell me if it works for you.

Dufgui commented 8 years ago

I don't see some change on master. Are you talking about the commit from previous week ? or maybe you not push it yet ?

I am just testing it, and my project not work with it. And when i add my PR, it's work. I think, it's due to our specific usage of emf. We split manualy to external proxy some of our work. In order to split files, to avoid to process huge file.

Dufgui commented 8 years ago

Now all my application unit test are ok. So if you have no more requirement i don"t evolve more my PR. Tks again for your help. And i hope you integrated it in the master branch

ghillairet commented 8 years ago

Could you give me more details about how you create containment proxies in your application. I don't see why the current master branch would fail your test cases.

I think the way to handle proxies is to do it like in this test https://github.com/emfjson/emfjson-jackson/blob/master/src/test/java/org/emfjson/jackson/junit/tests/ContainmentTest.java#L209

You should use references that do not resolve proxies.

ghillairet commented 8 years ago

Added in 0.15 release.