-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Improve the way Equals and Hashcode are generated. #150
Comments
Agreed, the current equals, hashCode and toString implementations functional but definitely not ideal for the reasons you've mentioned. They're basically the easiest implementation (which for a lot of people is good enough) but appending properties explicitly would be better. I don't see any technical reason why we shouldn't do this, it just needs an investment of effort. |
I would be willing to take it on. What is your prefered development process? Ideally I could create a different branch and when youre satisfied you could pull it in to your dev branch. |
The best way to work on this kind of change is to create a branch in your own fork and then create a pull request when you're done: https://help.github.com/articles/fork-a-repo |
I will just say, this is not a trivial change to take on and I fear it may not be the best change to tackle as your first contribution. Of course you're welcome to give this a go though and if you're willing to take the time then you'll no doubt learn a lot about CodeModel in the process. |
I forked the code,and I see why you say this will not be a trivial change. I am thinking that CodeModel will be the least of my worries. It looks like a fairly straightforward api. However looking at private void addHashCode(JDefinedClass jclass) {
JMethod hashCode = jclass.method(JMod.PUBLIC, int.class, "hashCode");
Class<?> hashcodeBuiler = ruleFactory.getGenerationConfig().isUseCommonsLang3() ?
org.apache.commons.lang3.builder.HashCodeBuilder.class :
org.apache.commons.lang.builder.HashCodeBuilder.class;
JBlock body = hashCode.body();
JInvocation reflectionHashCode = jclass.owner().ref(hashcodeBuiler).staticInvoke("reflectionHashCode");
reflectionHashCode.arg(JExpr._this());
body._return(reflectionHashCode);
hashCode.annotate(Override.class);
} I can not see any way from within the context of this function to get at the current set of json schema attributes for the class. Ideally if we had a class definition like: {
"type":"object",
"primary-key":["property1","property2"],
"properties":{
"property1":{"type":"string"}
"property2":{"type":"string"}
"property3":{"type":"string"}
}
} We could get at the attribute "primary-key", to decide whether to use the default reflectionhashcode strategy or to use a more efficient and selective strategy for hashcode and equals. |
I think you should keep these two things separate:
The former can be implemented and applied as a complete replacement for the reflection-based approach. The latter is an orthogonal change that requires the introduction of new extension properties. Regarding the current set of json schema attributes, the method ObjectRule#apply is passed the complete schema as a JsonNode. It would be worth stepping through this code and inspecting the values passed into each method so that you can understand what data is available. One problem here is that property names in the json schema may not map directly to Java field names. You don't want to reimplement the various rules that might be applied here so I recommend you use the fields on the generated type as your guide instead of the json schema. |
Any progress on this enhancement? |
I created a pull request with this enhancement #241. I tested it and it performs much better. |
Closed by #241. |
The current implementation of jsonschema2pojo generates the following for hashCode:
and
These will fail if a security manager is running and in addition they are not as performant as if you had used the following style:
Also it would be nice if there were a schema extension support to allow one to indicate which fields should participate in the hashCode and equals computation and which ones should not.
The text was updated successfully, but these errors were encountered: