Return generated idn on publish #61
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideThis PR updates the publish workflow to return the generated database identifier (idn), propagates it through the EventBus, extends the Event model to support idn, aligns build configurations to a release version, and adjusts test examples accordingly. Sequence diagram for publishing an event with generated idn propagationsequenceDiagram
participant "EventBusMessageBroker"
participant "Publisher"
participant "Database"
participant "EventBus"
"EventBusMessageBroker"->>"Publisher": publish(event, dataSource, topic)
"Publisher"->>"Database": INSERT INTO postevent.<topic> ... RETURNING idn
"Database"-->>"Publisher": idn
"Publisher"-->>"EventBusMessageBroker": idn
"EventBusMessageBroker"->>"EventBus": publish("events.<topic>", event.withIdn(idn))
Entity relationship diagram for event publishing and idn generationerDiagram
EVENT {
id VARCHAR
source VARCHAR
type VARCHAR
datacontenttype VARCHAR
dataschema VARCHAR
subject VARCHAR
data BLOB
time TIMESTAMP
idn BIGINT
topic VARCHAR
traceparent VARCHAR
}
EVENT ||..|| "postevent.<topic>" : contains
"postevent.<topic>" {
idn BIGINT PK
}
EVENT }|..|{ "postevent.<topic>" : published_to
Updated class diagram for Event and PublisherclassDiagram
class Event {
+String id
+String source
+String type
+String datacontenttype
+String dataschema
+String subject
+Object data
+Instant time
+Long idn
+String topic
+String traceparent
+static Event create(...)
+Event withIdn(Long idn)
}
class Publisher {
+static Long publish(Event event, Connection connection, String topic)
+static Long publish(Event event, DataSource ds, String topic)
}
Event <.. Publisher: uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of returning null when no generated key is found, consider throwing an exception or returning an Optional so missing IDs aren’t silently dropped.
- Use stmt.getLong(1) (or getLong("idn")) rather than getObject and casting to Long to avoid potential ClassCastExceptions with certain JDBC drivers.
- Since your SQL includes a RETURNING clause, the Statement.RETURN_GENERATED_KEYS flag may be redundant—consider simplifying to a normal prepareStatement(sql) and rely solely on RETURNING behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of returning null when no generated key is found, consider throwing an exception or returning an Optional<Long> so missing IDs aren’t silently dropped.
- Use stmt.getLong(1) (or getLong("idn")) rather than getObject and casting to Long to avoid potential ClassCastExceptions with certain JDBC drivers.
- Since your SQL includes a RETURNING clause, the Statement.RETURN_GENERATED_KEYS flag may be redundant—consider simplifying to a normal prepareStatement(sql) and rely solely on RETURNING behavior.
## Individual Comments
### Comment 1
<location> `core/src/main/java/com/p14n/postevent/Publisher.java:71` </location>
<code_context>
*/
- public static void publish(Event event, Connection connection, String topic) throws SQLException {
+ public static Long publish(Event event, Connection connection, String topic) throws SQLException {
if (topic == null || topic.trim().isEmpty()) {
throw new IllegalArgumentException("Topic name cannot be null or empty");
}
</code_context>
<issue_to_address>
**suggestion:** Topic validation logic is inconsistent between overloads.
Unify topic validation logic across both overloads to ensure consistent behavior.
Suggested implementation:
```java
public static Long publish(Event event, Connection connection, String topic) throws SQLException {
validateTopic(topic);
String sql = String.format("INSERT INTO postevent.%s (%s) VALUES (%s) RETURNING idn",
topic, SQL.CORE_COLS, SQL.CORE_PH);
try (PreparedStatement stmt = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)) {
setEventOnStatement(stmt, event);
```
```java
private static void validateTopic(String topic) {
if (topic == null || topic.trim().isEmpty()) {
throw new IllegalArgumentException("Topic name cannot be null or empty");
}
if (!topic.matches("^[a-z_]+$")) {
throw new IllegalArgumentException("Topic name must contain only lowercase letters and underscores");
}
}
```
You must also update any other overloads of `publish` in this file to use `validateTopic(topic);` instead of their own topic validation logic.
</issue_to_address>
### Comment 2
<location> `vertx/src/test/java/com/p14n/postevent/vertx/example/VertxConsumerExample.java:46` </location>
<code_context>
"text",
null,
"test",
- "hello".getBytes(), Instant.now(),1L ,"order",null));
+ "hello".getBytes(), Instant.now(),null ,"order",null));
client.subscribe("order", message -> {
</code_context>
<issue_to_address>
**suggestion (testing):** No test added to verify that the generated idn is returned and propagated.
Please update the test to assert that the idn is generated, returned by publish, and included in the event received by the consumer.
Suggested implementation:
```java
"text",
null,
"test",
"hello".getBytes(), Instant.now(),null ,"order",null));
// Capture the returned idn from publish
String publishedIdn = client.publish(
"text",
null,
"test",
"hello".getBytes(), Instant.now(),null ,"order",null
);
// Assert that the idn is generated
assertNotNull(publishedIdn, "Published idn should not be null");
// Use an AtomicReference to capture the received idn in the consumer
AtomicReference<String> receivedIdn = new AtomicReference<>();
client.subscribe("order", message -> {
System.out.println("Got message");
// Extract idn from the received message/event
String eventIdn = message.getIdn(); // Adjust this if your message object uses a different getter
receivedIdn.set(eventIdn);
// Assert that the received idn matches the published idn
assertEquals(publishedIdn, eventIdn, "Received idn should match published idn");
```
```java
"text",
null,
"test",
"hello".getBytes(), Instant.now(),null ,"order",null));
latch.await(10, TimeUnit.SECONDS);
// Final assertion after latch to ensure propagation
assertEquals(publishedIdn, receivedIdn.get(), "idn should be propagated to consumer");
```
- Ensure that `client.publish` returns the idn. If it does not, update its implementation accordingly.
- Ensure that the `message` object in the consumer has a `getIdn()` method or equivalent to extract the idn.
- Make sure `assertNotNull` and `assertEquals` are statically imported from your test framework (e.g., JUnit).
- Add `AtomicReference<String> receivedIdn = new AtomicReference<>();` at the appropriate scope if not already present.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| */ | ||
| public static void publish(Event event, Connection connection, String topic) throws SQLException { | ||
| public static Long publish(Event event, Connection connection, String topic) throws SQLException { | ||
| if (topic == null || topic.trim().isEmpty()) { |
Contributor
There was a problem hiding this comment.
suggestion: Topic validation logic is inconsistent between overloads.
Unify topic validation logic across both overloads to ensure consistent behavior.
Suggested implementation:
public static Long publish(Event event, Connection connection, String topic) throws SQLException {
validateTopic(topic);
String sql = String.format("INSERT INTO postevent.%s (%s) VALUES (%s) RETURNING idn",
topic, SQL.CORE_COLS, SQL.CORE_PH);
try (PreparedStatement stmt = connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)) {
setEventOnStatement(stmt, event); private static void validateTopic(String topic) {
if (topic == null || topic.trim().isEmpty()) {
throw new IllegalArgumentException("Topic name cannot be null or empty");
}
if (!topic.matches("^[a-z_]+$")) {
throw new IllegalArgumentException("Topic name must contain only lowercase letters and underscores");
}
}You must also update any other overloads of publish in this file to use validateTopic(topic); instead of their own topic validation logic.
| "text", | ||
| null, | ||
| "test", | ||
| "hello".getBytes(), Instant.now(),1L ,"order",null)); |
Contributor
There was a problem hiding this comment.
suggestion (testing): No test added to verify that the generated idn is returned and propagated.
Please update the test to assert that the idn is generated, returned by publish, and included in the event received by the consumer.
Suggested implementation:
"text",
null,
"test",
"hello".getBytes(), Instant.now(),null ,"order",null));
// Capture the returned idn from publish
String publishedIdn = client.publish(
"text",
null,
"test",
"hello".getBytes(), Instant.now(),null ,"order",null
);
// Assert that the idn is generated
assertNotNull(publishedIdn, "Published idn should not be null");
// Use an AtomicReference to capture the received idn in the consumer
AtomicReference<String> receivedIdn = new AtomicReference<>();
client.subscribe("order", message -> {
System.out.println("Got message");
// Extract idn from the received message/event
String eventIdn = message.getIdn(); // Adjust this if your message object uses a different getter
receivedIdn.set(eventIdn);
// Assert that the received idn matches the published idn
assertEquals(publishedIdn, eventIdn, "Received idn should match published idn"); "text",
null,
"test",
"hello".getBytes(), Instant.now(),null ,"order",null));
latch.await(10, TimeUnit.SECONDS);
// Final assertion after latch to ensure propagation
assertEquals(publishedIdn, receivedIdn.get(), "idn should be propagated to consumer");- Ensure that
client.publishreturns the idn. If it does not, update its implementation accordingly. - Ensure that the
messageobject in the consumer has agetIdn()method or equivalent to extract the idn. - Make sure
assertNotNullandassertEqualsare statically imported from your test framework (e.g., JUnit). - Add
AtomicReference<String> receivedIdn = new AtomicReference<>();at the appropriate scope if not already present.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Return database-generated idn in publish calls, propagate it through the EventBus, provide an Event.withIdn helper, and update project versions to 1.3.3.
New Features:
Build:
Tests: