phax / jcodemodel

A heavily extended fork of the com.sun.codemodel (from 2013/09)
Other
93 stars 34 forks source link

Add a method to remove annotations from JDefinedClass or JFieldVar #85

Closed ptahchiev closed 4 years ago

ptahchiev commented 4 years ago

Much like this guy here:

https://stackoverflow.com/questions/34874419/how-to-remove-an-annotation-on-a-jdefinedclass

I'm stuck trying to remove an annotation from a JFieldVar and I don't see any method. Can you please consider a method for removing an annotation. Thanks a ton.

guiguilechat commented 4 years ago

This should not be possible.

The base idea of the project, is that you ADD information in the JCM. Then you can refer to the information you added. Example where I create a classs that extends Map and so I add @SuppressWarning("serials")

var annot = creationClazz.annotate(SuppressWarnings.class);
...
annot.param("serial");

What would happen if I remove annotation between the annotate() and the param() ? It's a way to possibly silently break your program (see monkey patch).

IMO the issue is more, than it's possible to remove JFieldVar from a JDefinedClass.

I think what you should do, is parameter the class that generates the class, so that it does not add the annotation, and then add the annotations by yourself.

However in the specific case you gave, adding parameters to annotations does not create a new one, so you can just get the already existing annotation, and just parameter it again to rewrite the value.

eg

var annotClass = (SuppressWarnings.class);
JAnnotationUse annot = clazz.getAnnotation(annotClass);
if(annot==null) annot=clazz.annotate(annotClass);
annot.param("serial");

Here I get the existing annotation, if needed I create it, and then I parameter it.

phax commented 4 years ago

I can indeed see a use case when removing stuff makes sense - especially when you implement something that uses a "plugin" mechanism. As such I will add the support for it. My goal is not to force a certain process to that library but instead make it at flexible as possible - it's not on me to decide on the use cases....

guiguilechat commented 4 years ago

If you have a plugin, that removes what another plugin added, you have a huge design issue. Especially the end result will be non-deterministic without a specification of the ordering of the plugins (something I addressed in the issue about external code). Another example is, if you allow to remove a JDefinedClass : what if you make code that rely on that class ? It will crash when compiling, without a single explanation of who did remove that class.

Also in that case, removing the annotation would not help. What he wants is, to overwrite the value of that annotation, something which is already doable with the code I proposed.

The only "fix" there may be, is to return the existing annotation instead of throwing an exception when trying to create an annotation that is already present. But just the same : you are overriding what another code made, and this can become non deterministic and/or create invisible bugs that will be very difficult to debug.

ptahchiev commented 4 years ago

If you have a plugin, that removes what another plugin added, you have a huge design issue.

Sometimes you don't have access to the plugin that generates the class, but you still want to change the created class. Imagine you have a WSDL and you have a jaxb plugin that creates 'wrong' model out of the WSDL. You don't have access to the wsdl because it is hosted on a URL that is not yours. You don't have access to the plugin that generates the classes. What other option do you have?

guiguilechat commented 4 years ago

You use a plugin that is fixed to create the "good" model.

Also in the case of the url given : it's useless. I literally told you what you do, and it does not require to remove what the "wrong" plugin did. In this case your request is useless.

ptahchiev commented 4 years ago

Hi @guiguilechat, i see your point but I don't think you are correct. Imagine the following situation: you build a plugin that creates some classes which cover the majority of cases. Then you give this plugin to your customers and now they want to customize the classes that are generated. So what you do is create a hook for them and allow your customers to register in the hook and receive the JDefinedClass and modify it to their taste. I think this is a perfectly valid case.

guiguilechat commented 4 years ago

No, you don't get my point, at all.

You gave a link in the OP. I explained that the use case presented in that link precisely is wrong, and does not require any modification to actually work.

Now you make another one, and I also tell you that in that case too the use case is bad. if your first plugin creates a JDefinedClass with a JVar "myvar", and a setter with body "this.myvar=param" , and then you modify that jdefinedclass in your hook by removing this field, the compilation will fail and you will not know why especially if compile and generation are in separate steps. If it's not a field but an annotation that is removed, then the compilation phase will succeed, but runtime bug will appear, eg if the methods generated rely on the presence of such an annotation on the class.

If you want to use the plugin partially, you create your own plugin that does the job correctly, or you ask the plugin creator to allow a parameter for it ; If you want to hack into the result of a plugin, then your design will fail at some point, most likely in a way you won't even be able to link to your changes or in a way the person in charge will rather rewrite everything from scratch, rather than lose his time trying to debug it, and remove most of code you made. In your "perfectly valid case" what you SHOULD instead do, is to create a hook to specify classes generation.

But again any how, in the case you provided this is just not needed at all. You are asking to create bugs for a use case that is not valid. THAT is my point.

ptahchiev commented 4 years ago

I can't take your explanation. Let's not do something because people may misuse it. Well that's the whole point of software - if you remove a field and it doesn't compile then you carry the consequences. As to the annotations - yes it may fail at runtime, but again you carry the consequences. But also this feature will be extremely useful if you want to remove annotations that will not cause problems when removed. One example - hibernate has Cache annotation. We are generating a data-model with more than 400 entities and we add it on top of every entity. Your suggestion would be "Create a configuration file with 400 entities and specify which one should be cached and which one not". My suggestion is - remove the Cache annotation from the ones you don't want to be cached.

guiguilechat commented 4 years ago

Except here you don't carry the consequences - someone else will have to deal with the consequences of having a bad design. Especially if you don't have access to the code/source/doc of the code generator and can't ensure what you are doing is not creating bug.

If will not be "extremely useful". It will remain badly designed and a bug fest.

No, my suggestion has nothing to do with hibernate. Also my suggestion has nothing to do with creating a configuration file. Also my suggestion has nothing to do with specification of which class should have which behavior.

As I wrote, the plugin you are using is obviously not designed to handle your specific case, so my suggestion for this kind of situation was : write and use your own "plugin". If you don't want to have "Cache" on every entity, just don't "add it on top of every entity" in the first place. So configure/rewrite your "plugin" to NOT add cache, and then add the annotations using exactly the selection you have now - since you want to be the one responsible for adding this specific annotation. Instead of "remove the Cache annotation from the ones you don't want to be cached", it is "add the Cache annotation on the ones you want to be cached".

If you want to be the one choosing which classes are annotated, then make yourself this one. Just explicitly make this role available in your code. Don't try to sneakily enforce that role and them claim you will "carry the consequences" of something you have hidden.

ptahchiev commented 4 years ago

i'm not carrying any consequences. By design our product puts @Cache annotation on every entity. Then we give the platform to more than 20 different clients to develop on top. Once in a blue moon someone decides that they don't need the @Cache annotation on entity User. Right now, there's no way to remove it. What i'm saying is "Let's have this feature and then people will decide if they want to remove the annotation and carry the consequences themselves". I have no way of knowing in future which entity they not need cached - that's why the default behaviour caches everything.

phax commented 4 years ago

I see the arguments on both sides - and I would say you both have your points.

Feel free to continue the discussion - I am nevertheless closing the issue.