Community
Participate
Working Groups
Build ID: I20080918-0100 Steps To Reproduce: 1.Create new JPA Project. 2.Create new Entity in the created JPA Project - New -> Other -> Entity 3.Give a name to the entity and press next. 4.When you add a new Entity Field, you can set it any Type you want, including an unexisting or invalid Type, and any Name including an invalid Name. Proposal: In my opinion it will be good if a warning is shown when there is no corresponding Class to the Type of the field. Also I think that the creation of the Entity should not be possible when an invalid Class name is set for entity field's Type or when an invalid name is set for entity field's Name (for ex. with a space in it). More information:
I agree that a warning would be helpful in the case of an unresolvable Type reference and that an invalid class Name should prevent the user from continuing. If possible, the Name validation should be done in the Entity Fields dialog as this will prevent an invalid entry at the earliest point.
Created attachment 115824 [details] Patch
(In reply to comment #2) > Created an attachment (id=115824) [details] > Patch > Some problems to note with the patch: - Primitives are being incorrectly flagged as errors - Primitive or object arrays, such as java.lang.Character[] or char[] are being incorrectly flagged as errors - For both of the above cases, they are being reported as identifier errors, which restricts progress in wizard - Some unused imports in the patch This change would need to be released by Oct 29th in order to be included for the 2.1 release.
Created attachment 116268 [details] Patch In this new patch I tried to fix the restriction problems. I hope this time it will be ok.
It looks like things are working well now. I think there are just a couple of minor items to fix: - For the warning message, change text to, " " " does not exist on the project classpath" - there is a file in the patch called EntityCheck, which I assume is not supposed to be a part of the patch. I need to review the code a bit more, but I think we are close. (In reply to comment #4) > Created an attachment (id=116268) [details] > Patch > > In this new patch I tried to fix the restriction problems. I hope this time it > will be ok. >
Created attachment 116512 [details] Patch The warning message is changed.
Neil asked me to review this since he was busy at EclipseWorld today :) One thing I noticed is references to 2 internal classes in EntityRowTableWizardSection which can be removed fairly easily. CompilerOptions.VERSION_1_5 can be replaced with org.eclipse.jdt.core.JavaCore.VERSION_1_5 which is public. And for FilteredTypesSelectionDialog I think you should be able to use the static method org.eclipse.jdt.ui.JavaUI.createTypeDialog instead of directly referencing this class. We can still get this checked in for 2.1 if we squeeze it in on Monday after this Friday's M3 build.
Created attachment 116628 [details] Patch I tried to fix the references to the 2 internal classes. I hope it's OK now :)
Ok, I wasn't looking very closely yesterday, I see several more problems with this patch. 1.In EntityDataModelProvider the validation is calling JavaConventions.validateFieldName, yet what you are validating is a Type. If you called JavaConventions.validateJavaTypeName instead you would not need all the array checking. But you would still need to handle primitives. You are really just getting lucky that the primitives are valid because they happen to be valid field names. 2.In EntityDataModelProvider use JavaCore instead of CompilerOptions 3.Integer sigType = Signature.getTypeSignatureKind(sig); 2 calls to this, make sigType an int instead, no need to be boxing here. 4.EntityDataModelProvider_invalidArgument; - you do not need this String, in EntityDataModelProvider.checkInputElementsTypeValidation I don't see any reason to be catching an IllegalArgumentException, you should just be checking for those cases instead. I think if you use validateJavaTypeName instead of validateFieldName this will no longer be needed, it will handle null, "", and any whitespace which look like the things that result in an IllegalArgumentException being thrown.
(In reply to comment #9) In EntityDataModelProvider I changed the CompilerOptions references with JavaCore ;) Still when I use the suggested JavaConventions.validateJavaTypeName method I still have to handle not only the primitives, but also array checking, because the method restricts them both, like previously used JavaConventions.validateFieldName method. And in order to check for primitives and array types I have to use the Signature.createTypeSignature method. (The Signature.createTypeSignature method throws an exception only if type is not an array or primitive type and there is an white space. If type is an array type it does not throws an exception and returns a signature not for the whole type, but for the part of the type before the white space. If it is a primitive with a white space it returns null) So after a type is passed to JavaConventions.validateJavaTypeName method and it returns an error status I have to check what caused the status and I still have to use Signature.createTypeSignature method to check if the error status for the type is because of primitive or array type.If it is then OK. The problem is that if the error status is caused from an white space in not an array or primitive type, then Signature.createTypeSignature method will throw an exception, which I have to handle. Other problem is that if the type is an array type with an white space then Signature.createTypeSignature will return a signature not for the whole type and I can't use this signature for validation, because it is not whole. If I create a signature to check for primitive or array type before checking the TypeName (as I did in the last patch), I will again need to handle the exception if there is an white space. So I think that JavaConventions.validateJavaTypeName method is better to be used, but I cannot see how can I escape the primitives, array check and exception handling.
How about if you go ahead and put another patch where you switch to use validateJavaTypeName? It does appear I am wrong about not having to handle primitives and array types separately. The main thing I didn't like there was catching an IllegalArgumentException and then displaying a message to the user. I would think we can check for these things before calling that method instead of catching the exception. Changing the target to 2.2 since we've run out of time to get this in to 2.1
The Signature.createTypeSignature() method seem to cast an exception in many cases and I think it'll be difficult to check for all of them before calling the method (for example the method casts an exception not only in null and "" , but also in many other cases -> int.## , long>@@ , byte[].<< , java.lang.Character[],%% and many others, when there is an invalid symbol after '.' , '>' , '<' , ',' , or '[' ) So isn't it better to catch the exception?
I've checked in this patch with a few changes including switching to use validateJavaTypeName(). I changed a little bit of the validation code, I think I've still hit all the cases. Let me know if you hit any I've missed.
*** Bug 272262 has been marked as a duplicate of this bug. ***
