From 219933ccb88b370cc4181f4f9aaac083936c96e0 Mon Sep 17 00:00:00 2001 From: Jacob Hansson Date: Thu, 4 Feb 2016 21:51:33 -0600 Subject: [PATCH] Non-empty @name not allowed --- .../neo4j/kernel/impl/messages/Messages.java | 62 ---------- .../kernel/impl/proc/FieldInjections.java | 4 +- .../impl/proc/MethodSignatureCompiler.java | 21 +++- .../neo4j/kernel/impl/proc/OutputMappers.java | 18 +-- .../neo4j/kernel/impl/proc/TypeMappers.java | 12 +- .../java/org/neo4j/messages/Messages.java | 107 ++++++++++++++++++ .../proc/MethodSignatureCompilerTest.java | 18 ++- .../kernel/impl/proc/OutputMappersTest.java | 9 +- .../impl/proc/ProcedureJarLoaderTest.java | 9 +- .../impl/proc/ReflectiveProcedureTest.java | 17 +-- .../ReflectiveProcedureWithArgumentsTest.java | 6 +- 11 files changed, 174 insertions(+), 109 deletions(-) delete mode 100644 community/kernel/src/main/java/org/neo4j/kernel/impl/messages/Messages.java create mode 100644 community/kernel/src/main/java/org/neo4j/messages/Messages.java diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/messages/Messages.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/messages/Messages.java deleted file mode 100644 index 1846a5d2c24c7..0000000000000 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/messages/Messages.java +++ /dev/null @@ -1,62 +0,0 @@ -/* - * Copyright (c) 2002-2016 "Neo Technology," - * Network Engine for Objects in Lund AB [http://neotechnology.com] - * - * This file is part of Neo4j. - * - * Neo4j is free software: you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation, either version 3 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - * - * You should have received a copy of the GNU General Public License - * along with this program. If not, see . - */ -package org.neo4j.kernel.impl.messages; - -/** - * This is a beach head - if you are implementing code that prints human language - * messages in any way, through errors, warnings, explanations or any other - * mechanism - please help expand, refactor, move and use this this class to do - * so. - * - * The intention of this is to slowly introduce a proper i18n message API, - * backed by the standard i18n support in Java. - */ -public interface Messages -{ - Message proc_invalid_return_type_description = - msg("Procedures must return a Stream of records, where a record is a concrete class " + - "that you define, with public non-final fields defining the fields in the record. " + - "If you'd like your procedure to return `%s`, you could define a record class like:\n" + - "public class Output {\n" + - " public %s out;\n" + - "}\n" + - "\n" + - "And then define your procedure as returning `Stream`."); - - Message proc_static_field_annotated_as_context = - msg("The field `%s` in the class named `%s` is annotated as a @Context field," + - "but it is static. @Context fields must be public, non-final and non-static," + - "because they are reset each time a procedure is invoked."); - - interface Message - { - String defaultMessage(); - } - - static String get( Message msg, Object ... args ) - { - return String.format( msg.defaultMessage(), args ); - } - - static Message msg(String defaultMessage) - { - return () -> defaultMessage; - } -} diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java index 102e03aee4549..adedc0e6320d5 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/FieldInjections.java @@ -30,10 +30,10 @@ import org.neo4j.kernel.api.exceptions.ProcedureException; import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.proc.CallableProcedure; -import org.neo4j.kernel.impl.messages.Messages; +import org.neo4j.messages.Messages; import org.neo4j.procedure.Context; -import static org.neo4j.kernel.impl.messages.Messages.proc_static_field_annotated_as_context; +import static org.neo4j.messages.Messages.proc_static_field_annotated_as_context; /** * Injects annotated fields with appropriate values. diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/MethodSignatureCompiler.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/MethodSignatureCompiler.java index 164e8dcc87d33..1b5d8e95056b7 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/MethodSignatureCompiler.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/MethodSignatureCompiler.java @@ -28,8 +28,13 @@ import org.neo4j.kernel.api.exceptions.ProcedureException; import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.proc.ProcedureSignature.FieldSignature; +import org.neo4j.messages.Messages; import org.neo4j.procedure.Name; +import static org.neo4j.messages.Messages.proc_argument_missing_name; +import static org.neo4j.messages.Messages.proc_argument_name_empty; +import static org.neo4j.messages.Messages.proc_unmappable_argument_type; + /** * Given a java method, figures out a valid {@link org.neo4j.kernel.api.proc.ProcedureSignature} field signature. * Basically, it takes the java signature and spits out the same signature described as Neo4j types. @@ -56,12 +61,17 @@ public List signatureFor( Method method ) throws ProcedureExcept if ( !param.isAnnotationPresent( Name.class ) ) { throw new ProcedureException( Status.Procedure.FailedRegistration, - "Argument at position %d in method `%s` is missing an `@%s` annotation. " + - "Please add the annotation, recompile the class and try again.", i, method.getName(), - Name.class.getSimpleName() ); + Messages.get( proc_argument_missing_name, i, method.getName(), + Name.class.getSimpleName()) ); } String name = param.getAnnotation( Name.class ).value(); + if( name.trim().length() == 0 ) + { + throw new ProcedureException( Status.Procedure.FailedRegistration, + Messages.get( proc_argument_name_empty, i, method.getName() ) ); + } + try { signature.add(new FieldSignature( name, typeMappers.neoTypeFor( type ) )); @@ -69,8 +79,9 @@ public List signatureFor( Method method ) throws ProcedureExcept catch ( ProcedureException e ) { throw new ProcedureException( e.status(), - "Argument `%s` at position %d in `%s` with type `%s` cannot be converted to a Neo4j type: %s", - name, i, method.getName(), param.getType().getSimpleName(), e.getLocalizedMessage() ); + Messages.get( proc_unmappable_argument_type, + name, i, method.getName(), param.getType().getSimpleName(), + e.getMessage() ) ); } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/OutputMappers.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/OutputMappers.java index 28c8b68fb8183..74c9852d22182 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/OutputMappers.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/OutputMappers.java @@ -33,13 +33,13 @@ import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.proc.ProcedureSignature; import org.neo4j.kernel.api.proc.ProcedureSignature.FieldSignature; -import org.neo4j.kernel.impl.messages.Messages; +import org.neo4j.messages.Messages; import static java.lang.reflect.Modifier.isPublic; import static java.lang.reflect.Modifier.isStatic; import static java.util.Arrays.asList; import static java.util.stream.Collectors.toList; -import static org.neo4j.kernel.impl.messages.Messages.proc_invalid_return_type_description; +import static org.neo4j.messages.Messages.proc_invalid_return_type_description; /** * Takes user-defined record classes, and does two things: Describe the class as a {@link ProcedureSignature}, and provide a mechanism to convert @@ -137,8 +137,7 @@ public OutputMapper mapper( Method method ) throws ProcedureException if ( cls != Stream.class ) { throw new ProcedureException( Status.Procedure.TypeError, - Messages.get( proc_invalid_return_type_description, - cls.getSimpleName(), cls.getSimpleName() )); + Messages.get( proc_invalid_return_type_description, cls.getSimpleName() )); } ParameterizedType genType = (ParameterizedType) method.getGenericReturnType(); @@ -195,15 +194,20 @@ private void assertIsValidRecordClass( Class userClass ) throws ProcedureExce || userClass.getPackage() != null && userClass.getPackage().getName().startsWith( "java." ) ) { throw new ProcedureException( Status.Procedure.TypeError, - Messages.get( proc_invalid_return_type_description, - userClass.getSimpleName(), userClass.getSimpleName() )); + Messages.get( proc_invalid_return_type_description, userClass.getSimpleName() )); } } private List instanceFields( Class userClass ) { return asList( userClass.getDeclaredFields() ).stream() - .filter( ( f ) -> !isStatic( f.getModifiers() ) && !f.getName().contains( "$" ) ) + .filter( ( f ) -> !isStatic( f.getModifiers() ) && + !isSynthetic( f.getModifiers() ) ) .collect( toList() ); } + + private static boolean isSynthetic( int modifiers ) + { + return (modifiers & 0x00001000) != 0; + } } diff --git a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/TypeMappers.java b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/TypeMappers.java index 5423845fdabb5..a3142622888f3 100644 --- a/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/TypeMappers.java +++ b/community/kernel/src/main/java/org/neo4j/kernel/impl/proc/TypeMappers.java @@ -29,6 +29,7 @@ import org.neo4j.kernel.api.exceptions.ProcedureException; import org.neo4j.kernel.api.exceptions.Status; import org.neo4j.kernel.api.proc.Neo4jTypes.AnyType; +import org.neo4j.messages.Messages; import static org.neo4j.kernel.api.proc.Neo4jTypes.NTAny; import static org.neo4j.kernel.api.proc.Neo4jTypes.NTBoolean; @@ -38,6 +39,7 @@ import static org.neo4j.kernel.api.proc.Neo4jTypes.NTMap; import static org.neo4j.kernel.api.proc.Neo4jTypes.NTNumber; import static org.neo4j.kernel.api.proc.Neo4jTypes.NTString; +import static org.neo4j.messages.Messages.proc_unmappable_type; public class TypeMappers { @@ -140,11 +142,11 @@ private NeoValueConverter toList( NeoValueConverter inner ) private ProcedureException javaToNeoMappingError( Type cls ) { - return new ProcedureException( Status.Statement.InvalidType, "Don't know how to map `%s` to the Neo4j Type " + - "System. Please refer to " + - "to the documentation for full details. For " + - "your reference, known types are: %s", - cls, Iterables.toList(javaToNeo.keySet()) ); + List types = Iterables.toList( javaToNeo.keySet() ); + types.sort( (a,b)->a.toString().compareTo( b.toString() ) ); + + return new ProcedureException( Status.Statement.InvalidType, + Messages.get( proc_unmappable_type, cls, types )); } public static class SimpleConverter implements NeoValueConverter diff --git a/community/kernel/src/main/java/org/neo4j/messages/Messages.java b/community/kernel/src/main/java/org/neo4j/messages/Messages.java new file mode 100644 index 0000000000000..4cd08f4f75d10 --- /dev/null +++ b/community/kernel/src/main/java/org/neo4j/messages/Messages.java @@ -0,0 +1,107 @@ +/* + * Copyright (c) 2002-2016 "Neo Technology," + * Network Engine for Objects in Lund AB [http://neotechnology.com] + * + * This file is part of Neo4j. + * + * Neo4j is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.neo4j.messages; + +import java.text.MessageFormat; + +/** + * This is a beach head - if you are implementing code that prints human language + * messages in any way, through errors, warnings, explanations or any other + * mechanism - please help expand, refactor, move and use this this class to do + * so. + * + * The intention of this is to slowly introduce a proper i18n message API, + * backed by the standard i18n support in Java. + * + * The messages in this file are templates fed to the {@link MessageFormat} formatter. + * If you are including things like times, dates or other locale-sensitive things, + * please use the pattern utilities provided by {@link MessageFormat} to format them, + * since that will make it much easier to internationalize them in the future. + */ +public interface Messages +{ + // Note: Single-quotes, `'` and curly brackets have special meaning + // to MessageFormat and need to be escaped as "''" and "'{'", respectively. + + // Note: This file is going to become very large. As it does, please use + // best judgement to split it into appropriate categories and sub-files. + + Message proc_invalid_return_type_description = + msg("Procedures must return a Stream of records, where a record is a concrete class\n" + + "that you define, with public non-final fields defining the fields in the record.\n" + + "If you''d like your procedure to return `{0}`, you could define a record class like:\n" + + "public class Output '{'\n" + + " public {0} out;\n" + + "'}'\n" + + "\n" + + "And then define your procedure as returning `Stream`."); + + Message proc_static_field_annotated_as_context = + msg("The field `{0}` in the class named `{1}` is annotated as a @Context field,\n" + + "but it is static. @Context fields must be public, non-final and non-static,\n" + + "because they are reset each time a procedure is invoked."); + + Message proc_unmappable_type = + msg("Don''t know how to map `{0}` to the Neo4j Type System.\n" + + "Please refer to to the documentation for full details.\n" + + "For your reference, known types are: {1}"); + + Message proc_unmappable_argument_type = + msg("Argument `{0}` at position {1} in `{2}` with\n" + + "type `{3}` cannot be converted to a Neo4j type: {4}" ); + + Message proc_argument_missing_name = + msg("Argument at position {0} in method `{1}` is missing an `@{2}` annotation.\n" + + "Please add the annotation, recompile the class and try again." ); + + Message proc_argument_name_empty = + msg("Argument at position {0} in method `{1}` is annotated with a name,\n" + + "but the name is empty, please provide a non-empty name for the argument." ); + + interface Message + { + String defaultMessage(); + } + + // Implementation note: + // We use this silly-looking indirection of calling Messages#get(..) + // instead of just calling message.defaultMessage() wherever we need + // these messages. The reason is the intended direction of this interface, + // which is that we'd be able to plug i18n lookups in underneath, so if + // you in the future are running Neo4j with a German locale, you'd get + // german messages out on the other end. + + /** + * Get a message, optionally filling in positional arguments. + * @param msg the message to output + * @param args optional dynamic parts of the message + * @return the message with the dynamic parameters inserted + */ + static String get( Message msg, Object ... args ) + { + return MessageFormat.format( msg.defaultMessage(), args ); + } + + /** Create a new message */ + static Message msg(String defaultMessage) + { + return () -> defaultMessage; + } +} diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/MethodSignatureCompilerTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/MethodSignatureCompilerTest.java index 4756b90b04008..74dd2c049ac26 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/MethodSignatureCompilerTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/MethodSignatureCompilerTest.java @@ -97,14 +97,12 @@ public void shouldGiveHelpfulErrorOnUnmappable() throws Throwable // Expect exception.expect( ProcedureException.class ); - exception.expectMessage( "Argument `name` at position 0 in `echoWithInvalidType` with type " + - "`UnmappableRecord` cannot be converted to a Neo4j type: Don't know how to map " + + exception.expectMessage( "Argument `name` at position 0 in `echoWithInvalidType` with\n" + + "type `UnmappableRecord` cannot be converted to a Neo4j type: Don't know how to map " + "`class org.neo4j.kernel.impl.proc.MethodSignatureCompilerTest$UnmappableRecord` to " + - "the Neo4j Type System. Please refer to to the documentation for full details. For " + - "your reference, known types are: [long, double, class java.lang.Number, class java" + - ".lang.Object, class java.lang.String, class java.lang.Long, class java.lang.Double," + - " interface java.util.List, interface java.util.Map, boolean, class java.lang" + - ".Boolean]" ); + "the Neo4j Type System.\n" + + "Please refer to to the documentation for full details.\n" + + "For your reference, known types are:" ); // When new MethodSignatureCompiler(new TypeMappers()).signatureFor( echo ); @@ -118,9 +116,9 @@ public void shouldGiveHelpfulErrorOnMissingAnnotations() throws Throwable // Expect exception.expect( ProcedureException.class ); - exception.expectMessage( "Argument at position 1 in method `echoWithoutAnnotations` is missing an " + - "`@Name` annotation. Please add the annotation, recompile the class and " + - "try again." ); + exception.expectMessage( "Argument at position 1 in method `echoWithoutAnnotations` is missing an `@Name` " + + "annotation.\n" + + "Please add the annotation, recompile the class and try again." ); // When new MethodSignatureCompiler(new TypeMappers()).signatureFor( echo ); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/OutputMappersTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/OutputMappersTest.java index 390356a2f6ff2..7d9888d68e8ed 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/OutputMappersTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/OutputMappersTest.java @@ -157,14 +157,13 @@ public void shouldWarnAgainstStdLibraryClassesSinceTheseIndicateUserError() thro // Expect exception.expect( ProcedureException.class ); - exception.expectMessage( "Procedures must return a Stream of records, where a record is a concrete class that you define, with public non-final " + - "fields defining the fields in the record. If you'd like your procedure to return `Long`, you could define a record class " + + exception.expectMessage( "Procedures must return a Stream of records, where a record is a concrete class\n" + + "that you define, with public non-final fields defining the fields in the record.\n" + + "If you'd like your procedure to return `Long`, you could define a record class " + "like:\n" + "public class Output {\n" + " public Long out;\n" + - "}\n" + - "\n" + - "And then define your procedure as returning `Stream`." ); + "}" ); // When mapper(Long.class); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ProcedureJarLoaderTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ProcedureJarLoaderTest.java index 3ed57f7b0da47..087cead421269 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ProcedureJarLoaderTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ProcedureJarLoaderTest.java @@ -117,8 +117,13 @@ public void shouldGiveHelpfulErrorOnInvalidProcedure() throws Throwable // Expect exception.expect( ProcedureException.class ); - exception.expectMessage( "A procedure must return a `java.util.stream.Stream`, " + - "`ClassWithInvalidProcedure.booleansAreNotAcceptableReturnTypes` returns `boolean`." ); + exception.expectMessage( "Procedures must return a Stream of records, where a record is a concrete class\n" + + "that you define, with public non-final fields defining the fields in the record.\n" + + "If you'd like your procedure to return `boolean`, you could define a record class " + + "like:\n" + + "public class Output {\n" + + " public boolean out;\n" + + "}\n" ); // When jarloader.loadProcedures( jar ); diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureTest.java index 2f8c3e3796708..69b79e972c849 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureTest.java @@ -188,13 +188,15 @@ public void shouldGiveHelpfulErrorOnProcedureReturningInvalidRecordType() throws { // Expect exception.expect( ProcedureException.class ); - exception.expectMessage( "Procedures must return a Stream of records, where a record is a concrete class that" + - " you define, with public non-final fields defining the fields in the record. If " + - "you'd like your procedure to return `String`, you could define a record class " + + exception.expectMessage( "Procedures must return a Stream of records, where a record is a concrete class\n" + + "that you define, with public non-final fields defining the fields in the record.\n" + + "If you'd like your procedure to return `String`, you could define a record class " + "like:\n" + "public class Output {\n" + " public String out;\n" + - "}" ); + "}\n" + + "\n" + + "And then define your procedure as returning `Stream`" ); // When compile( ProcedureWithInvalidRecordOutput.class ).get( 0 ); @@ -206,8 +208,9 @@ public void shouldGiveHelpfulErrorOnContextAnnotatedStaticField() throws Throwab // Expect exception.expect( ProcedureException.class ); exception.expectMessage( "The field `gdb` in the class named `ProcedureWithStaticContextAnnotatedField` is " + - "annotated as a @Context field,but it is static. @Context fields must be public, " + - "non-final and non-static,because they are reset each time a procedure is invoked." ); + "annotated as a @Context field,\n" + + "but it is static. @Context fields must be public, non-final and non-static,\n" + + "because they are reset each time a procedure is invoked." ); // When compile( ProcedureWithStaticContextAnnotatedField.class ).get( 0 ); @@ -223,8 +226,6 @@ public void shouldAllowNonStaticOutput() throws Throwable assertEquals( 1, proc.signature().outputSignature().size() ); } - - public static class MyOutputRecord { public String name; diff --git a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureWithArgumentsTest.java b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureWithArgumentsTest.java index 47aba8b738449..d4ba3b205a11b 100644 --- a/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureWithArgumentsTest.java +++ b/community/kernel/src/test/java/org/neo4j/kernel/impl/proc/ReflectiveProcedureWithArgumentsTest.java @@ -102,9 +102,9 @@ public void shouldFailIfMissingAnnotations() throws Throwable { // Expect exception.expect( ProcedureException.class ); - exception.expectMessage( "Argument at position 0 in method `listCoolPeople` is missing an " + - "`@Name` annotation. Please add the annotation, recompile the class " + - "and try again." ); + exception.expectMessage( "Argument at position 0 in method `listCoolPeople` " + + "is missing an `@Name` annotation.\n" + + "Please add the annotation, recompile the class and try again." ); // When compile( ClassWithProcedureWithoutAnnotatedArgs.class );