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

Support for nested binding #240

Closed
vans239 opened this issue Oct 5, 2022 · 14 comments
Closed

Support for nested binding #240

vans239 opened this issue Oct 5, 2022 · 14 comments

Comments

@vans239
Copy link

vans239 commented Oct 5, 2022

I am interested in ability to bind exisiting instance recursively.
Based on my understanding of source code the feature currently is not supported

Kotlin example

@CompiledJson
class Container {
    var a: String = "a"
    var b: String = "c"
    var c: String = "b"
}

@CompiledJson
class Example {
    var value: Int = 1
    var container: Container = Container()
}

Example_DslJsonConverter contains something like:

public void bindContent(...){
			...
			instance.setValue(com.dslplatform.json.NumberConverter.deserializeInt(reader));
			...
			instance.setContainer(reader_container().read(reader));
			
}

Instead I'd like to have:

public void bindContent(...){
			...
			instance.setValue(com.dslplatform.json.NumberConverter.deserializeInt(reader));
			...
                        binder_container().bind(reader, instance.getContainer())
}

That way instance reading would be zero allocation (mostly)

@plokhotnyuk
Copy link
Contributor

@vans239 Here is how encoders and decoders are derived recursively with the library extension for Scala.

Probably similar approach can be used with Kotlin.

@vans239
Copy link
Author

vans239 commented Oct 5, 2022

@plokhotnyuk I am not sure that follow your idea.
Dsl-json supports nested objects converters from the box. It also supports 'binding' of json into the instance, but it doesn't work recursively. The recursive support would require code generation change (aka annotation processor)

@zapov
Copy link
Member

zapov commented Oct 5, 2022

I probably didn't add support for this because its not obvious how to handle nested binding when not all properties are provided.
You would need to do some reset() before the binding... but I guess this could be enforced by having some dsl-json signature to implement that behavior.

@vans239
Copy link
Author

vans239 commented Oct 5, 2022

Would it be reasonable thing to add to the library? Thinking on prototyping this.

You would need to do some reset() before the binding

For auto-generated bind indeed values should be reset. Good point.
For provided BindObject it should be responsibility of handling in bind method.

The alternative - require caller to reset values before binding.

@zapov
Copy link
Member

zapov commented Oct 5, 2022

It is a reasonable thing. And looks useful to add.
Not sure when tho. I have some other changes pending too, just never the time to implement them :(

@zapov
Copy link
Member

zapov commented Oct 5, 2022

If you want to make a PR that would certainly move it faster :)

@vans239
Copy link
Author

vans239 commented Oct 5, 2022

I had yesterday an issue when tried to experiment: mvn install on clean checkout fails due to tests.
Similar to #102 (comment)

java.lang.AssertionError: expected:<NOTE> but was:<ERROR>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:118)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at com.dslplatform.json.DslValidationTest.checkNonNull(DslValidationTest.java:129)

Is there any chance that you know what could cause it? I can share full env setup later today

@zapov
Copy link
Member

zapov commented Oct 5, 2022

Without digging into I suspect it might be due to missing Mono.
There are tests running the DSL Platform compiler on a subset of cases.
You can exclude that project if you don't want to have Mono around.

@vans239
Copy link
Author

vans239 commented Dec 16, 2022

@zapov I made some proof of concept PR #243. Will appreciate feedback

@zapov
Copy link
Member

zapov commented May 30, 2023

Tnx for the PR.
I found some time to work on DSL-JSON, went through your PR and merged a working version: b8a12fa
It seems there was no need for this reset I had on mind, as one can do that in the bind method.
Also, I dont think there is a need for mandatory, nullable guards as we can just pass null into the method and one can just decide what to do (either call into read or take some shared instance and bind that instead).

Let me know if you think there should be some changes/improvements.

Hopefully I managed to wrap up all the things I planned and make a new v2 version based on Java8

@vans239
Copy link
Author

vans239 commented May 30, 2023

That's amazing.
I'll close my PR as it's not needed anymore.
I'll try out the feature when v2 beta is published to maven.

one can just decide what to do (either call into read or take some shared instance and bind that instead)

I have some concerns about shared instance / pool, because that would make it tricky to use same binder multiple times in one event.
So if instance not provided only safe choice is to allocate

@zapov
Copy link
Member

zapov commented Jun 11, 2023

v2 released. While this does support wrapper classes there is still some work for actual binding of instances, but thats a different feature

@zapov zapov closed this as completed Jun 11, 2023
@vans239
Copy link
Author

vans239 commented Jun 12, 2023

Could you point to v2 release?
I don't see it in artifactory https://mvnrepository.com/artifact/com.dslplatform/dsl-json and releases page https://github.com/ngs-doo/dsl-json/releases

@zapov
Copy link
Member

zapov commented Jun 12, 2023

https://repo1.maven.org/maven2/com/dslplatform/dsl-json/2.0.0/

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

No branches or pull requests

3 participants