Conversation
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey @p14n - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| } | ||
|
|
||
| private long findLatestMessageId(Connection connection, String topicName) throws SQLException { | ||
| String sql = "SELECT MAX(idn) FROM postevent." + topicName; |
There was a problem hiding this comment.
🚨 suggestion (security): Consider sanitizing or validating topicName in SQL construction.
Directly concatenating topicName into the SQL string may pose SQL injection risks if the value is not strictly controlled. Although it may be validated elsewhere, adding safeguards here could improve security.
Suggested implementation:
private long findLatestMessageId(Connection connection, String topicName) throws SQLException {
if (!isValidTopicName(topicName)) {
throw new IllegalArgumentException("Invalid topic name: " + topicName);
}
String sql = "SELECT MAX(idn) FROM postevent." + topicName;
try (PreparedStatement stmt = connection.prepareStatement(sql)) { private boolean isValidTopicName(String topicName) {
return topicName != null && topicName.matches("^[a-zA-Z0-9_]+$");
}Ensure that the isValidTopicName method is placed within the CatchupService class along with other helper methods. Adjust the regular expression as needed to match the naming conventions for topic names in your environment.
| public void onMessage(SystemEvent message) { | ||
| if (Objects.requireNonNull(message) == SystemEvent.CatchupRequired) { | ||
| oneAtATime(() -> catchup(message.topic), () -> onMessage(message)); | ||
| } else if (message == SystemEvent.FetchLatest) { |
There was a problem hiding this comment.
issue (complexity): Consider extracting responsibilities in fetchLatest into separate methods for topic validation, transaction handling, business logic, and event publishing.
It would be beneficial to extract some of the responsibilities in fetchLatest into separate methods. For example, you can:
- Extract the null check. Move topic-name validation into its own method.
- Separate DB transaction handling. Isolate the logic to start a transaction, perform operations, and commit/rollback into a helper.
- Extract business logic. Isolate request-specific logic like getting the current HWM, fetching events, and processing them.
- Delegate event publishing. Move triggering follow-up events to a dedicated method.
This will reduce nesting and make unit testing easier. For instance, you could refactor as follows:
private boolean isValidTopic(String topicName) {
if (topicName == null) {
LOGGER.warn("Topic name is null for fetch latest");
return false;
}
return true;
}
private int performFetchLatest(Connection conn, String topicName) throws SQLException {
long currentHwm = getCurrentHwm(conn, topicName);
long latestMessageId = catchupServer.getLatestMessageId(topicName);
if (latestMessageId <= currentHwm) {
LOGGER.info("No new messages found after HWM {} for topic {}", currentHwm, topicName);
return 0;
}
List<Event> events = catchupServer.fetchEvents(latestMessageId - 1, latestMessageId, 1, topicName);
if (events.isEmpty()) {
LOGGER.info("No events found in range for topic: {}", topicName);
return 0;
}
return writeEventsToMessagesTable(conn, events);
}
private void triggerCatchupIfNeeded(String topicName, long currentHwm, long latestMessageId) {
if (latestMessageId > currentHwm) {
systemEventBroker.publish(SystemEvent.CatchupRequired.withTopic(topicName));
}
}
private int fetchLatest(String topicName) {
if (!isValidTopic(topicName)) return 0;
try (Connection conn = datasource.getConnection()) {
conn.setAutoCommit(false);
int processedCount = performFetchLatest(conn, topicName);
conn.commit();
triggerCatchupIfNeeded(topicName, getCurrentHwm(conn, topicName), catchupServer.getLatestMessageId(topicName));
LOGGER.info("Processed latest event for topic {}", topicName);
return processedCount;
} catch (SQLException e) {
LOGGER.error("Failed to fetch latest", e);
return 0;
}
}These changes keep the current functionality intact while reducing the method's complexity and making future maintenance easier.
Fixes #34
Summary by Sourcery
Introduce a mechanism to periodically fetch the single latest event for each topic, ensuring recent data is processed quickly before a full catchup occurs.
New Features:
Bug Fixes:
call->select).Enhancements:
Deployment:
db.t4g.microinstance class andgp3storage type.