spring-attic / jdbc

Apache License 2.0
11 stars 18 forks source link

'payload.' prefix invalidates my column SpEL expression #16

Closed restjohn closed 6 years ago

restjohn commented 7 years ago

https://github.com/spring-cloud-stream-app-starters/jdbc/blob/e72160b34b59a753c62cf023922187d6688214ec/spring-cloud-starter-stream-sink-jdbc/src/main/java/org/springframework/cloud/stream/app/jdbc/sink/JdbcSinkConfiguration.java#L91

I want to use the following column expression in my JDBC Sink starter:

geom: new org.postgis.PGgeometry(new org.postgis.Point(payload['geom']))

Unfortunately, when I use that expression, the code linked above results in an exception:

2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] Caused by: org.springframework.beans.BeanInstantiationException: Failed to instantiate [org.springframework.integration.jdbc.JdbcMessageHandler]: Circular reference involving containing bean 'org.springframework.cloud.stream.app.jdbc.sink.JdbcSinkConfiguration' - consider declaring the factory method as static for independence from its containing instance. Factory method 'jdbcMessageHandler' threw exception; nested exception is org.springframework.expression.spel.SpelParseException: EL1041E: After parsing a valid expression, there is still more data in the expression: 'org'
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:189) ~[spring-beans-4.3.10.RELEASE.jar!/:4.3.10.RELEASE]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at org.springframework.beans.factory.support.ConstructorResolver.instantiateUsingFactoryMethod(ConstructorResolver.java:588) ~[spring-beans-4.3.10.RELEASE.jar!/:4.3.10.RELEASE]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[na:1.8.0_131]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at org.springframework.cloud.stream.app.jdbc.sink.JdbcSinkConfiguration$$EnhancerBySpringCGLIB$$6697df27.jdbcMessageHandler(<generated>) ~[spring-cloud-starter-stream-sink-jdbc-1.2.0.RELEASE.jar!/:1.2.0.RELEASE]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at org.springframework.expression.common.TemplateAwareExpressionParser.parseExpression(TemplateAwareExpressionParser.java:60) ~[spring-expression-4.3.10.RELEASE.jar!/:4.3.10.RELEASE]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at org.springframework.cloud.stream.app.jdbc.sink.JdbcSinkConfiguration.jdbcMessageHandler(JdbcSinkConfiguration.java:91) ~[spring-cloud-starter-stream-sink-jdbc-1.2.0.RELEASE.jar!/:1.2.0.RELEASE]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at org.springframework.cloud.stream.app.jdbc.sink.JdbcSinkConfiguration$$EnhancerBySpringCGLIB$$6697df27$$FastClassBySpringCGLIB$$ee2a3359.invoke(<generated>) ~[spring-cloud-starter-stream-sink-jdbc-1.2.0.RELEASE.jar!/:1.2.0.RELEASE]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[na:1.8.0_131]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] at org.springframework.beans.factory.support.SimpleInstantiationStrategy.instantiate(SimpleInstantiationStrategy.java:162) ~[spring-beans-4.3.10.RELEASE.jar!/:4.3.10.RELEASE]
2017-10-02T07:26:45.709-06:00 [APP/PROC/WEB/0] [OUT] ... 42 common frames omitted

What is the purpose of adding a second, payload.-prefixed version of the same expression? This seems superfluous to me, and in my case prevents me from using the JDBC Sink starter.

restjohn commented 7 years ago

I would classify this more as a bug, myself, as there is no workaround as far as I can see.

garyrussell commented 7 years ago

I agree this is a bug; the expression should make no assumptions that the column comes from a payload property, which is what "payload." + value does, even though that is, by far, the most common use case (clearly, since nobody has reported this to-date).

The column could come from a header headers['foo'] or be something more complex, as has been reported.

This is going to be tricky to fix, however, without breaking all existing apps using the current assumption.