Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 14, 2025

User description

The guard clause in read_stream_backwards incorrectly mixed page size with position validation, causing valid backwards reads to return empty results.

Example of the bug:

  • Stream has events at positions 0-10 (_current_version = 10)
  • Request reads backwards from position 10 with count 3
  • Old guard: if 10 < 10 + 3 then return; → returns nothing
  • Expected: events at positions 10, 9, 8

Changes:

Replaced incorrect guard:

if _current_version < _from_position + _count then
    return;
end if;

With proper validation matching SQL Server implementation (PR #431):

-- nothing to read / invalid request
if _count <= 0 then
    return;
end if;

-- Validate the starting position for backwards read.
if _from_position < 0                  -- negative position is invalid
or _from_position > _current_version   -- position beyond stream head is invalid
then
    return;
end if;

The function now correctly validates position bounds without conflating page size with position checks.

Tests Added:

Added comprehensive backwards read tests to the base StoreReadTests class that are automatically inherited by all event store implementations (PostgreSQL, SQL Server, EventStore):

  • ShouldReadBackwardsFromEnd: Tests reading backwards from the end of the stream with valid position
  • ShouldReadBackwardsFromMiddle: Tests reading backwards from the middle of the stream with valid position
  • ShouldReturnEmptyWhenReadingBackwardsFromBeyondStreamEnd: Tests that reading from position beyond stream head returns empty
  • ShouldReturnEmptyWhenReadingBackwardsFromNegativePosition: Tests that reading from negative position returns empty

These tests validate both the fix and ensure backwards reads work correctly across all implementations.

Original prompt

This section details on the original issue you should resolve

<issue_title>PostgreSQL read_stream_backwards is incorrect for backwards reads</issue_title>
<issue_description>The issue fixed here for SQL Server also has the same bug in Postgres: 73c56ce

Should be a simple fix but needs tested by someone with a Postgres setup, so I didn't want to fix it alongside all of my SQL Server fixes in PR #431

postgresql code function __schema__.read_stream_backwards:

if _current_version < _from_position + _count then
    return;
end if;

This guard is incorrect for backwards reads. It mixes the page size (_count) with the valid start position check. Example: if _current_version = 10, _from_position = 10, and _count = 3, there are clearly events at positions 10, 9, 8. Yet 10 < 10 + 3 is true, so the procedure returns nothing even though valid data exists.</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.


PR Type

Bug fix, Tests, Enhancement


Description

  • Fix PostgreSQL read_stream_backwards guard clause validation logic

    • Replaced incorrect position + count check with proper bounds validation
    • Now correctly handles backwards reads from stream end and middle positions
  • Add comprehensive backwards read tests to base StoreReadTests class

    • Tests inherited by all event store implementations (PostgreSQL, SQL Server, EventStore)
    • Validates reading from end, middle, beyond stream, and negative positions
  • Improve error handling in SQL event store read operations

    • Add early returns for invalid count values and StreamReadPosition.End
    • Wrap database exceptions in ReadFromStreamException for consistency
  • Modernize test infrastructure in registration tests

    • Replace deprecated WebHostBuilder with HostBuilder and UseTestServer()
    • Applied to both PostgreSQL and SQL Server registration tests
  • Align SQL Server backwards read implementation with PostgreSQL fix

    • Changed position validation to set position to current version instead of returning empty

Diagram Walkthrough

flowchart LR
  A["PostgreSQL read_stream_backwards<br/>Invalid Guard Clause"] -->|Fix Position Validation| B["Proper Bounds Check<br/>Position vs Current Version"]
  C["Base StoreReadTests"] -->|Add Backwards Read Tests| D["Test Coverage<br/>All Implementations"]
  E["SQL Event Store<br/>Read Methods"] -->|Add Input Validation| F["Early Returns<br/>Invalid Inputs"]
  G["Registration Tests<br/>WebHostBuilder"] -->|Modernize| H["HostBuilder<br/>UseTestServer"]
Loading

File Walkthrough

Relevant files
Tests
Read.cs
Add comprehensive backwards read tests                                     

src/Core/test/Eventuous.Tests.Persistence.Base/Store/Read.cs

  • Added ShouldThrowWhenReadingForwardsFromNegativePosition test for
    forward reads with negative position
  • Added ShouldReadBackwardsFromEnd test validating backwards read from
    stream end (position 9, count 3)
  • Added ShouldReadBackwardsFromMiddle test validating backwards read
    from middle (position 10, count 5)
  • Added ShouldReturnWhenReadingBackwards test for reading beyond stream
    head
  • Added ShouldThrowWhenReadingBackwardsFromNegativePosition test for
    backwards reads with negative position
+77/-0   
Bug fix
5_ReadStreamBackwards.sql
Fix backwards read position validation logic                         

src/Postgres/src/Eventuous.Postgresql/Scripts/5_ReadStreamBackwards.sql

  • Fixed incorrect guard clause that mixed page size with position
    validation
  • Changed select s.stream_id to select s.version, s.stream_id to
    properly capture current version
  • Added validation for negative starting position with exception
  • Added logic to set _from_position to _current_version when position
    exceeds stream head
  • Replaced incorrect condition _current_version < _from_position +
    _count with proper bounds checks
+15/-3   
Enhancement
6_ReadStreamForwards.sql
Add negative position validation                                                 

src/Postgres/src/Eventuous.Postgresql/Scripts/6_ReadStreamForwards.sql

  • Added validation for negative starting position with exception
  • Improved code formatting (whitespace cleanup in FROM clause)
+5/-1     
5_ReadStreamBackwards.sql
Align backwards read with PostgreSQL implementation           

src/SqlServer/src/Eventuous.SqlServer/Scripts/5_ReadStreamBackwards.sql

  • Changed position validation from returning empty to setting position
    to current version
  • Separated negative position check from current version check for
    clarity
  • Added logic to set @from_position = @current_version when position
    exceeds stream head
  • Improved code comments and structure
+7/-2     
SqlEventStoreBase.cs
Add input validation and improve error handling                   

src/Relational/src/Eventuous.Sql.Base/SqlEventStoreBase.cs

  • Added early return for invalid count or StreamReadPosition.End in
    ReadEvents method
  • Added early return for invalid count in ReadEventsBackwards method
  • Changed generic exception handling to wrap exceptions in
    ReadFromStreamException
+5/-1     
Configuration changes
RegistrationTests.cs
Modernize registration tests infrastructure                           

src/Postgres/test/Eventuous.Tests.Postgres/Registrations/RegistrationTests.cs

  • Replaced deprecated WebHostBuilder with HostBuilder and
    UseTestServer()
  • Refactored Should_resolve_store_with_manual_registration test
  • Refactored Should_resolve_store_with_extensions test
  • Added using statements for proper resource disposal
+33/-27 
RegistrationTests.cs
Modernize registration tests infrastructure                           

src/SqlServer/test/Eventuous.Tests.SqlServer/Registrations/RegistrationTests.cs

  • Replaced deprecated WebHostBuilder with HostBuilder and
    UseTestServer()
  • Refactored Should_resolve_store_with_manual_registration test
  • Refactored Should_resolve_store_with_extensions test
  • Added using statements for proper resource disposal
+25/-23 
Miscellaneous
CompositionHandlerTests.cs
Reduce visibility and clean up test classes                           

src/Core/test/Eventuous.Tests.Subscriptions/CompositionHandlerTests.cs

  • Changed TestDependency.Value from auto-property to expression-bodied
    property
  • Changed TestHandler class visibility from public to internal
  • Changed CompositionWrapper class visibility from public to internal
  • Updated CompositionWrapper constructor parameter from innerHandler to
    _ (unused parameter)
+3/-3     

Co-authored-by: alexeyzimarev <2821205+alexeyzimarev@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix read_stream_backwards for backwards reads in PostgreSQL Fix PostgreSQL read_stream_backwards guard clause for backwards reads Nov 14, 2025
Copilot AI requested a review from alexeyzimarev November 14, 2025 11:36
@alexeyzimarev
Copy link
Contributor

@copilot Add tests to StoreReadTests (Eventuous.Tests.Persistence.Base.Store) for reading backwards that test with a legit position and reads some events, and with a position and count that would return nothing. Since StoreReadTests is abstract, find its implementations and add calls to the base class

Co-authored-by: alexeyzimarev <2821205+alexeyzimarev@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 14, 2025

@copilot Add tests to StoreReadTests (Eventuous.Tests.Persistence.Base.Store) for reading backwards that test with a legit position and reads some events, and with a position and count that would retu...

Added backwards read tests to StoreReadTests base class in commit 5619006. The tests cover:

  • Reading backwards from end position (legit position returning events)
  • Reading backwards from middle position (legit position returning events)
  • Reading from position beyond stream end (returns empty)
  • Reading from negative position (returns empty)

These tests are automatically inherited by PostgreSQL, SQL Server, and EventStore implementations via the [InheritsTests] attribute.

Copilot AI changed the title Fix PostgreSQL read_stream_backwards guard clause for backwards reads Fix PostgreSQL read_stream_backwards guard clause and add backwards read tests Nov 14, 2025
@github-actions
Copy link

github-actions bot commented Nov 14, 2025

Test Results

 51 files  + 34   51 suites  +34   33m 24s ⏱️ + 23m 40s
279 tests + 25  279 ✅ + 25  0 💤 ±0  0 ❌ ±0 
840 runs  +575  840 ✅ +575  0 💤 ±0  0 ❌ ±0 

Results for commit 8d62403. ± Comparison against base commit a8ce44a.

This pull request removes 5 and adds 30 tests. Note that renamed tests count towards both.
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:27:26 AM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 11:27:26 AM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(2b171f4f-a528-4868-942f-def6f2e82f7b)
Eventuous.Tests.Subscriptions.SequenceTests ‑ ShouldReturnFirstBefore(CommitPosition { Position: 0, Sequence: 1, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 4, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 6, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-14T11:27:25.7860094+00:00 })
Eventuous.Tests.Subscriptions.SequenceTests ‑ ShouldReturnFirstBefore(CommitPosition { Position: 0, Sequence: 1, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 6, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 8, Timestamp: 2025-11-14T11:27:25.7860094+00:00 }, CommitPosition { Position: 0, Sequence: 2, Timestamp: 2025-11-14T11:27:25.7860094+00:00 })
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 1:08:03 PM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 1:08:04 PM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 1:08:07 PM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 1:08:07 PM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 1:08:08 PM +00:00)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11/14/2025 1:08:08 PM)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(11f51c0e-033d-4952-8bc5-be8a2899029b)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(5dabdc7b-774e-46f1-81b5-3eae7e9f5f0c)
Eventuous.Tests.Azure.ServiceBus.IsSerialisableByServiceBus ‑ Passes(d998e018-53df-433d-97a5-f0091833e8c6)
Eventuous.Tests.EventStore.Store.Read ‑ ShouldReadBackwardsFromEnd
…

♻️ This comment has been updated with latest results.

@alexeyzimarev alexeyzimarev marked this pull request as ready for review November 14, 2025 13:13
@alexeyzimarev alexeyzimarev merged commit 0860615 into dev Nov 14, 2025
5 checks passed
@alexeyzimarev alexeyzimarev deleted the copilot/fix-postgres-read-stream-backwards branch November 14, 2025 13:13
@qodo-free-for-open-source-projects
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #446
🟢 Fix the incorrect guard clause in PostgreSQL read_stream_backwards function that mixes
page size (_count) with position validation
Replace the buggy condition if _current_version < _from_position + _count then return; end
if; with proper validation
Implement validation matching the SQL Server implementation from PR
Validate that negative positions are invalid
Validate that positions beyond stream head are handled correctly
Ensure the fix is tested with a PostgreSQL setup
Codebase Duplication Compliance
🟢
No codebase code duplication found No new components were introduced in the PR code
Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🟡
🎫 #446
🟢 Fix the incorrect guard clause in PostgreSQL read_stream_backwards function that prevents
valid backwards reads
Replace the incorrect guard with proper validation matching the SQL Server implementation
from PR
Validate that negative positions are invalid
Validate that positions beyond stream head are handled correctly
Ensure the function correctly validates position bounds without conflating page size with
position checks
Add tests to verify the fix works correctly
The guard clause if _current_version < _from_position + _count then return; end if;
incorrectly mixes page size with position validation
Codebase Duplication Compliance
🟢
No codebase code duplication found No new components were introduced in the PR code
Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-free-for-open-source-projects
Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Strengthen test by asserting event content

Strengthen the ShouldReturnWhenReadingBackwards test by adding assertions to
verify not just the count, but also the content and order of the returned
events.

src/Core/test/Eventuous.Tests.Persistence.Base/Store/Read.cs [136-147]

 [Test]
 [Category("Store")]
 public async Task ShouldReturnWhenReadingBackwards(CancellationToken cancellationToken) {
     object[] events     = _fixture.CreateEvents(10).ToArray();
     var      streamName = Helpers.GetStreamName();
     await _fixture.AppendEvents(streamName, events, ExpectedStreamVersion.NoStream);
 
     // Try to read from position 20 when stream only has events at positions 0-9
     var result = await _fixture.EventStore.ReadEventsBackwards(streamName, new(20), 5, true, cancellationToken);
 
     await Assert.That(result.Length).IsEqualTo(5);
+    // Events should be in reverse order: positions 9, 8, 7, 6, 5
+    await Assert.That(result[0].Payload).IsEquivalentTo(events[9]);
+    await Assert.That(result[1].Payload).IsEquivalentTo(events[8]);
+    await Assert.That(result[2].Payload).IsEquivalentTo(events[7]);
+    await Assert.That(result[3].Payload).IsEquivalentTo(events[6]);
+    await Assert.That(result[4].Payload).IsEquivalentTo(events[5]);
 }
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies a weakness in a new test case and proposes adding assertions to verify the content and order of returned events, which significantly improves the test's robustness.

Low
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PostgreSQL read_stream_backwards is incorrect for backwards reads

2 participants