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 builder classes for immutable objects #22

Closed
azell opened this issue Dec 31, 2016 · 29 comments
Closed

Support builder classes for immutable objects #22

azell opened this issue Dec 31, 2016 · 29 comments

Comments

@azell
Copy link

azell commented Dec 31, 2016

I would like to add dsl-json support to https://github.com/soabase/soabase-halva/blob/master/halva/src/main/java/io/soabase/halva/caseclass/README.md . halva generates immutable classes that are paired with builder classes. Jackson support for the builder pattern can be implemented with @JsonDeserialize.

Example: https://github.com/soabase/soabase-halva/blob/master/examples/example-generated/JsonTestCase.java

Is there an equivalent way to specify a custom builder object with dsl-json? Default compilation results in:

CompiledJson requires accessible public no argument constructor, therefore 'com.dslplatform.maven.Example.ExampleCase' requires public no argument constructor
@zapov
Copy link
Member

zapov commented Dec 31, 2016

Currently no, but it's probably not that hard to add support for it.

The current workaround is to use type or property converter and implement such builder there, but that's not really a generic friendly way. And since the helper classes would have to be public... it's not a nice workaround.

I've been postponing adding something along the line of builder to the CompiledJson annotation; that would solve you'r problem.
I guess I can look into adding it sooner, rather than later :)

@zapov
Copy link
Member

zapov commented Dec 31, 2016

So, I've looked at this... and...

Supporting such builders with just a few annotations doesn't seem likely (due to various issues).
I did add an example to show off how to do it in a similar way using DTOs (plus one improvements to the processor which helps with the setup). You can take a look here: https://github.com/ngs-doo/dsl-json/blob/master/examples/Maven/src/main/java/com/dslplatform/maven/ImmutablePerson.java

@zapov
Copy link
Member

zapov commented Feb 11, 2018

FWIW this is now supported via reflection (no need for annotations, just matching signature): https://github.com/ngs-doo/dsl-json/blob/master/examples/Runtime/src/main/java/com/dslplatform/runtime/Example.java#L26
Although it does require public ctor so not sure it will work with your example.

If someone wants to further improve immutables, basic analysis is done in: https://github.com/ngs-doo/dsl-json/blob/master/java8/src/main/java/com/dslplatform/json/runtime/ImmutableAnalyzer.java

@zapov
Copy link
Member

zapov commented Mar 14, 2018

This now works via annotation processor as long as ctor is public.
To support it exactly like that Builder class needs to be referenced from CompiledJson

@ratcashdev
Copy link

What if the immutable properties are in a superclass? e.g.:

public abstract class Parent {
  private final String x;
  protected Parent(String x) {
    this.x = x;
  }
}

public class Child extends Parent {
  public Child (String x) {
    super(x);
  }
}

@zapov
Copy link
Member

zapov commented Jul 16, 2018

I don't think it should matter. Can you write a test for that? Not sure if one exists. It will be useful if it works or if it doesn't work :)

@ratcashdev
Copy link

The following code fails to compile with 1.7.5.

public class NotWorkingHierarchy {

    @CompiledJson
    public static abstract class Person {

        private final String name;

        protected Person(String name) {
            this.name = name;
        }

        public String getName() {
            return name;
        }
    }

    // this is the culprit
    @CompiledJson
    public abstract static class Parent extends Person {

        public Parent(String name) {
            super(name);
        }
    }

    @CompiledJson
    public static class Mother extends Parent {

        @CompiledJson
        public Mother(String name) {
            super(name);
        }
    }

    @CompiledJson
    public static class Father extends Parent {

        @CompiledJson
        public Father(String name) {
            super(name);
        }
    }

}

@zapov
Copy link
Member

zapov commented Jul 17, 2018

I see. Tnx. Will add support for that in next version

@zapov
Copy link
Member

zapov commented Aug 4, 2018

@ratcashdev your example should work in v1.8.0

@anantharaman93
Copy link

anantharaman93 commented Sep 4, 2018

Hai. Is there any example on working example of dsl-json with lombok/autovalue when using @CompiledJson (I am using builder pattern)

@zapov
Copy link
Member

zapov commented Sep 4, 2018

@suraj1291993 sorry, I don't have one. But I can certainly help you resolve issues if you try to build one

@anantharaman93
Copy link

anantharaman93 commented Sep 5, 2018

Hai, this is the lombok code, I am trying. But during compilation, dsl-json code is not getting generated.

@CompiledJson
@Builder(toBuilder = true)
@Getter
@ToString
public class Emplyee {
	private String name;
	private int id;
}

For similar object constructed using AutoValue

@CompiledJson
@AutoValue
public abstract class Employee {
	public abstract String name();
	public abstract int id();
	public static Builder builder() {
		return new AutoValue_Employee.Builder();
	}
	@AutoValue.Builder
	public abstract static class Builder {
		public abstract Builder name(String name);
		public abstract Builder id(int id);
		public abstract Employee build();
	}
}

I am getting the following exception during compile time

Error:(6, 1) java: Abstract class (com.test.Employee) is referenced, but it doesn't have registered implementations with @CompiledJson. At least one concrete extension of specified Abstract class must be annotated with CompiledJson annotation or allow unknown types during analysis

I am using Java 10,0.1 in intellij

@zapov
Copy link
Member

zapov commented Sep 5, 2018

Hm... actually the builder pattern feature is still not supported, hence this issue is still open.

I would guess the problem here is that Lombok is either running after the dsl-json or not copying the annotation to the concrete target.
It would help if you submit a PR with the Lombok example, although it will probably have to wait for the builder pattern feature to fully work.

@anantharaman93
Copy link

You can find the decompiled class here

Can I know when you are planning to support builder pattern ?

@zapov
Copy link
Member

zapov commented Sep 5, 2018

I'll look that it goes into 1.8.1 which hopefully will be done by this months end.

@zapov
Copy link
Member

zapov commented Sep 15, 2018

Compile time support for builder pattern is working now. I don't plan on adding runtime support for that (at least not now).
Relevant tests: https://github.com/ngs-doo/dsl-json/blob/master/tests-java8/src/test/java/com/dslplatform/json/BuilderTest.java

@anantharaman93
Copy link

anantharaman93 commented Sep 16, 2018

Hai, I wanted to try the latest development setup. But I am getting following exception. (I was using AutoValue)

Error:(34, 32) java: Builder pattern is not available with current analysis setup

@zapov
Copy link
Member

zapov commented Sep 16, 2018

You are probably trying to use DSL annotation processor. Try with Java8 version:

<dependency>
  <groupId>com.dslplatform</groupId>
  <artifactId>dsl-json-java8</artifactId>
  <version>1.8.1</version>
</dependency>

@anantharaman93
Copy link

I am having dsl-json-1.8.1, dsl-json-java8-1.8.1 & dsl-json-processor-1.8.1 in my class path. I added dsl-json-1.8.1 also in path because of CompiledJson annotation. The code I am using is

@CompiledJson(formats = {CompiledJson.Format.ARRAY, CompiledJson.Format.OBJECT})
@Builder(toBuilder = true)
@Getter
@ToString
public class Employee {
	@JsonAttribute(index = 1, name = "name")
	private String name;
	@JsonAttribute(index = 2, name = "id")
	private int id;
}

When I am compiling, I am getting following errors.

Error:java: Unable to find matching property: 'arg0' used in constructor. Either use annotation processor on source code, on bytecode with -parameters flag (to enable parameter names) or manually create an instance via converter
Error:java: Unable to find matching property: 'arg1' used in constructor. Either use annotation processor on source code, on bytecode with -parameters flag (to enable parameter names) or manually create an instance via converter

and

Error:(9, 1) java: 'com.test.Employee' does not have an empty or matching constructor since it has @CompiledJson annotation.
Error:(9, 1) java: No matching constructors found for 'com.test.Employee'. Make sure there is at least one matching constructor available.
Error:(9, 1) java: 'com.test.Employee' requires public no argument constructor since it has @CompiledJson annotation.
Error:(9, 1) java: Array format is not supported in the DSL compiler. Found on: 'com.test.Employee'.

Also, when I am using auto-value,

@CompiledJson(formats = {CompiledJson.Format.ARRAY, CompiledJson.Format.OBJECT})
@AutoValue
public abstract class Employee {
	@JsonAttribute(index = 1, name = "name")
	public abstract String getName();
	@JsonAttribute(index = 2, name = "id")
	public abstract int getId();
	public static Builder builder() {
		return new AutoValue_Employee.Builder();
	}
	@AutoValue.Builder
	public abstract static class Builder {
		public abstract Builder setName(String newName);
		public abstract Builder setId(int newId);
		public abstract Employee build();
	}
}

I am getting this error

Error:(36, 32) java: Builder pattern is not available with current analysis setup. Use annotation processor which supports such feature
Error:(22, 1) java: Array format is not supported in the DSL compiler. Found on: 'com.test.Employee'.

Kindly, guide me on how should I change my code.

@zapov
Copy link
Member

zapov commented Sep 17, 2018

Remove the dsl-json-processor dependency. Thats for the DSL annotation processor. You don't need that since there is a more feature rich version in the dsl-json-java8 dependency.

@anantharaman93
Copy link

I tried your suggestion, The autovalue example works fine. Thanks. For the lombok example, I am getting the error

Error:java: Unable to find matching property: 'arg0' used in constructor. Either use annotation processor on source code, on bytecode with -parameters flag (to enable parameter names) or manually create an instance via converter
Error:java: Unable to find matching property: 'arg1' used in constructor. Either use annotation processor on source code, on bytecode with -parameters flag (to enable parameter names) or manually create an instance via converter

and

Error:(10, 1) java: 'com.test.Employee' does not have an empty or matching constructor since it has @CompiledJson annotation.

@anantharaman93
Copy link

Also, I tried using FreeBuilder. I am getting an error

Error:(19, 1) java: Abstract class (com.test.Employee) is referenced, but it doesn't have registered implementations with @CompiledJson. At least one concrete extension of specified Abstract class must be annotated with CompiledJson annotation or allow unknown types during analysis

The code snippet I used is

@CompiledJson(formats = {CompiledJson.Format.ARRAY, CompiledJson.Format.OBJECT})
@FreeBuilder
public abstract class Employee {
	@JsonAttribute(index = 1, name = "name")
	public abstract String name();
	@JsonAttribute(index = 2, name = "id")
	public abstract int id();
	public static Builder builder() {
		return new Builder();
	}
	public static class Builder extends Employee_Builder {
	}
}

@zapov
Copy link
Member

zapov commented Sep 17, 2018

For Lombok I assume annotation processor is not run and it falls back to runtime probing since you've initialized DslJson using (Settings.withRuntime or something alike)
I know Lombok doesn't use annotation processors, but some special method of generating code. You should look if there is a way to force DslJson annotation processor to run after the Lombok.

Can you show me the generated code for this FreeBuilder?

@anantharaman93
Copy link

You can find generated code for FreeBuilder here

@zapov
Copy link
Member

zapov commented Sep 17, 2018

Does this FreeBuilder works if you do

public static Employee_Builder builder() {
  return new Builder();
}

I can look into recognizing this kind of relationship during analysis.
Currently it expects the same type from builder

@anantharaman93
Copy link

Even after trying suggestion, I am getting the same exception

@zapov
Copy link
Member

zapov commented Sep 17, 2018

It would be great if you can add such test here: https://github.com/ngs-doo/dsl-json/blob/master/tests-java8/src/test/java/com/dslplatform/json/BuilderTest.java#L106 and make a PR.
I can look into making it work after.
You should write code similar to what would be generated by FreeBuilder (like I did in the linked example)

@zapov
Copy link
Member

zapov commented Sep 17, 2018

@suraj1291993 it should be working now: https://github.com/ngs-doo/dsl-json/blob/master/tests-java8/src/test/java/com/dslplatform/json/BuilderTest.java#L153

@zapov
Copy link
Member

zapov commented Sep 21, 2018

@azell FWIW this is now working in latest version. It's sufficient to add @CompiledJson to class declaration

@zapov zapov closed this as completed Sep 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants