Not up to standards ⛔

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review

Overview

This PR does two things: adds TEXT OID 25 support to PostgresType (fixing a real interoperability bug with Npgsql 10), and adds a new e2e-csharp/ module with 7 xUnit tests that validate the PostgreSQL wire protocol from a C# client. Both are well-motivated and the core fix is correct.

PostgresType.java - Positive

• Adding TEXT(25, String.class, -1, value -> value) is the right fix. OID 25 is indeed the PostgreSQL TEXT type, and Npgsql 10 changed to send string parameters as OID 25 instead of OID 1043 (VARCHAR).
• The case VARCHAR, TEXT -> grouping in deserializeBinary correctly reuses the same logic for both OIDs.
• deserializeText already uses CODE_MAP.get(code) and calls type.textParser.apply(str), so TEXT OID 25 is handled there too once it's in the enum - no extra case needed.
• The generic type fix (Map map -> Map<?, ?> map with explicit (Map<String, ?>) map cast) is a proper cleanup of raw-type warnings, and the @SuppressWarnings placement on the enclosing method is correct.

PostgresType.java - Concerns

• getTypeForValue never returns TEXT - this is intentional and correct (ArcadeDB should encode outgoing strings as VARCHAR/OID 1043, not TEXT), but a comment explaining the asymmetry would help future maintainers understand why TEXT exists for deserialization only.

C# E2E Tests - Positive

• Test structure mirrors e2e-python / e2e-js nicely.
• IAsyncLifetime with proper DisposeAsync is the right pattern for Testcontainers.NET.
• Using Server Compatibility Mode=NoTypeLoading;No Reset On Close=true in the connection string is the correct Npgsql configuration for non-standard PostgreSQL backends.
• Good spread of test scenarios: basic connection, simple query, parameterized select, multiple params, transaction.

C# E2E Tests - Issues

1. Silent exception swallowing in ParameterizedInsert

catch (NpgsqlException)
{
    // ArcadeDB sends a RowDescription after INSERT which Npgsql rejects...
}

Catching all NpgsqlException silently is risky. If the INSERT fails for a completely different reason (network error, auth error, SQL syntax error), the test still appears to pass because the SELECT might find data from a previous run. Consider at minimum logging the caught exception, or checking ex.SqlState to confirm it's the expected protocol deviation. Better yet, this protocol non-compliance (RowDescription after INSERT) should have a follow-up issue filed so it can be fixed in ArcadeDB rather than worked around in tests.

2. Test data isolation - hardcoded IDs across shared fixture

All tests insert records with hardcoded IDs (ci1, ci2, ps1, mp1, pi1, tx1) into the shared NpgsqlTest type. xUnit runs test classes sequentially by default but methods within a class can interleave, and CI retry will fail with duplicate key violations. Consider using a unique prefix per test run (e.g., a Guid.NewGuid() prefix) or INSERT ... IF NOT EXISTS if ArcadeDB supports it.

3. ContainerBuilder(image) constructor

The Testcontainers.NET idiomatic API is:

new ContainerBuilder()
    .WithImage(image)
    ...

Using new ContainerBuilder(image) where the image is a constructor argument is unconventional and may not be documented. If the CI passes this is a non-blocking style note, but .WithImage(image) is clearer and aligns with documented examples.

4. Missing test artifact upload in CI

The job generates test-results.trx but never uploads it:

- name: E2E C# Tests
  run: dotnet test --logger "trx;LogFileName=test-results.trx"

Add an upload step (especially with if: always()) so failures are diagnosable:

- name: Upload test results
  if: always()
  uses: actions/upload-artifact@...
  with:
    name: csharp-e2e-test-results
    path: e2e-csharp/ArcadeDB.E2ETests/TestResults/

Test Coverage

The new PostgresTypeTest additions are solid:

• deserializeTextOid25AsString - text format
• deserializeBinaryOid25AsString - binary format
• textTypeCodeIs25 - OID constant
• additions to isArrayTypeFalse, typeCodesAreCorrect, typeSizesAreCorrect

The removed unused imports (ParameterizedTest, Arguments, MethodSource, Stream) are clean-up, not loss of coverage.

Summary

The core bug fix is solid and the test additions are welcome. Address the silent exception swallow and test data isolation before merge.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.30%. Comparing base (953354c) to head (1571e6f).
⚠️ Report is 6 commits behind head on main.

@@           Coverage Diff           @@
##             main    #4038   +/-   ##
=======================================
  Coverage   64.29%   64.30%           
=======================================
  Files        1598     1598           
  Lines      120766   120786   +20     
  Branches    25729    25734    +5     
=======================================
+ Hits        77647    77668   +21     
+ Misses      32372    32366    -6     
- Partials    10747    10752    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Code Review

Overview

This PR adds TEXT OID 25 support to PostgresType (fixing a real Npgsql 10 interoperability bug), and introduces a new e2e-csharp/ module with 7 xUnit tests validating the PostgreSQL wire protocol from a C# client. The core fix is correct and well-motivated.

PostgresType.java - Positive

• Adding TEXT(25, String.class, -1, value -> value) is the right fix. OID 25 is the standard PostgreSQL TEXT type; Npgsql 10 changed to send string parameters as OID 25 instead of OID 1043 (VARCHAR), causing the "Type with code 25 not supported" error on all parameterized queries.
• case VARCHAR, TEXT -> in deserializeBinary cleanly groups the two types with identical handling.
• getTypeFromArcade continues to return VARCHAR for outgoing column descriptions - this is correct; TEXT OID 25 only needs to be recognized when receiving parameters from the client, not when sending column metadata to it.
• Moving @SuppressWarnings("unchecked") from parseArrayFromString (where it was no longer needed) to serializeAsText (where the (Map<String, ?>) map cast is) is a cleanup improvement.
• The removal of the unused DateUtils import is a nice cleanup.
• 100% diff coverage on the Java changes per Codacy.

Issues and Suggestions

1. ParameterizedInsert silently swallows a known protocol bug

try { await insert.ExecuteNonQueryAsync(); }
catch (NpgsqlException)
{
    // ArcadeDB sends a RowDescription after INSERT which Npgsql rejects; the write
    // still commits, so verify via SELECT rather than failing here.
}

This hides a real ArcadeDB server-side protocol violation (sending RowDescription in response to an INSERT). While the comment is honest, silently catching any NpgsqlException here means the test will also pass silently if the INSERT fails for a completely different reason (e.g., type mismatch, connection error, etc.) - making the follow-up SELECT the only real assertion. Consider:

• Opening a GitHub issue to track the RowDescription-after-INSERT bug.
• Using a more targeted catch, or at minimum asserting a specific exception message/type.
• Adding an [Issue("...")] attribute or //TODO: tracked in #XXXX to make this technically-expected failure traceable.

2. Test isolation - shared mutable state

All 7 tests share a single NpgsqlTest document type with no cleanup between runs. Tests use hardcoded IDs ('ci1', 'ci2', 'ps1', 'mp1', 'pi1', 'tx1'). On a re-run (e.g., flaky CI retry) the SELECT assertions in most tests will pass trivially because records from the prior run are still there, even if the INSERT just failed. A DELETE FROM NpgsqlTest or unique ID strategy (e.g., Guid.NewGuid()) per test run would make these tests reliable on retries.

3. MultipleParameters is too permissive

select.CommandText = "SELECT FROM NpgsqlTest WHERE name = $1 AND value = $2";
select.Parameters.AddWithValue("Alice");
select.Parameters.AddWithValue("100");
await using var reader = await select.ExecuteReaderAsync();
Assert.True(await reader.ReadAsync());

This will match any record with name = 'Alice' and value = '100', including those inserted by CreateTypeAndInsert (ci1) and ParameterizedSelect (ps1). The test does not isolate its own data, so it can't distinguish between "parameters worked" and "previous tests left matching rows". Filter on id = 'mp1' after the INSERT to make this meaningful.

4. CI job missing test result upload

The Python and JS e2e jobs upload test results as artifacts for debugging failures. The new C# job generates test-results.trx but does not upload it:

- name: Upload test results
  uses: actions/upload-artifact@...
  if: always()
  with:
    name: csharp-test-results
    path: e2e-csharp/ArcadeDB.E2ETests/TestResults/

Consider adding this for consistency with the other e2e jobs, and to make CI failures debuggable.

5. getTypeFromArcade does not cover TEXT for completeness (minor)

getTypeFromArcade maps all ArcadeDB string types to VARCHAR. This is intentional for outgoing column metadata. However, a comment explaining that TEXT is receive-only (client-initiated OID) would save the next reader from wondering why TEXT is never returned here.

6. GITHUB_TOKEN env var in C# job is unused

The C# job sets GITHUB_TOKEN in the env, but unlike the Testcontainers Java jobs (which may pull from GHCR), the C# Testcontainers library reads ARCADEDB_DOCKER_IMAGE directly and the image is already loaded from cache. The token is not needed and adds unnecessary noise - though it's harmless.

Test Coverage - Positive

The three new PostgresTypeTest unit tests (deserializeTextOid25AsString, deserializeBinaryOid25AsString, textTypeCodeIs25) are well-targeted and use the project's preferred assertThat(...).isEqualTo(...) style. The additions to isArrayTypeFalse, typeCodesAreCorrect, and typeSizesAreCorrect are consistent with existing patterns.

Summary

The Java fix (TEXT OID 25) is correct, minimal, and well-tested - this should unblock Npgsql 10 users immediately. The C# e2e module is a valuable addition but has some test reliability concerns (shared state, swallowed exceptions, weak assertions) that should be addressed to avoid flaky CI. The most important issue is #1: the silently caught NpgsqlException in ParameterizedInsert masking a real server-side bug that should be tracked separately.

I haven't seen this PR, I closed the issue with some fix and tests. Feel free to add more tests if you need it.

Code Review

Overview

This PR adds a C# E2E test suite using xUnit + Testcontainers + Npgsql 10, mirrors the existing e2e-python structure, and adds minor Java cleanups in PostgresType.java (raw-type fixes and @SuppressWarnings placement). The TEXT OID 25 enum member appears to have already been present in main; the PR adds test coverage for it and confirms it works end-to-end via Npgsql.

Protocol Compliance - Two Workarounds Mask Real Bugs

ParameterizedInsert silently swallows a protocol error:

This hides a server-side bug - ArcadeDB is sending a RowDescription message after an INSERT, which violates the PostgreSQL wire protocol (INSERT should return CommandComplete, not a row result). Any real Npgsql application doing parameterized inserts will get unexpected exceptions. The workaround is acceptable for a test, but the underlying server issue should be tracked and fixed separately.

Transaction test bypasses the Npgsql transaction API:

ArcadeDB's PostgreSQL handler should accept the standard BEGIN [TRANSACTION] [ISOLATION LEVEL ...] syntax. Rejecting it means any Npgsql application that opens a real transaction will fail. Again, pragmatic for a test but should be filed as a bug.

Test Design

Test isolation is fragile. Tests insert rows with hardcoded IDs (ci1, ps1, mp1, pi1, tx1) into a shared NpgsqlTest type with no cleanup. The fixture creates the type once and all tests write into it. This is why CreateTypeAndInsert uses Assert.True(count >= 2) instead of Assert.Equal(2, count) - it is already defending against data accumulation from other tests. Consider using random/UUID-based IDs per test, or adding per-test cleanup.

SimpleQuery asserts HasRows against schema:types - this always returns builtin types so it will never be empty; the assertion is not testing anything meaningful. Checking reader.FieldCount > 0 or reading at least one row would be more informative.

CI Workflow

Missing NuGet package cache. The Python job uses cache: 'pip'. The C# job can use the built-in cache support in setup-dotnet.

Missing artifact upload for test results. The js-e2e-tests job uploads test artifacts. The C# job generates a .trx file but never uploads it, making failure debugging harder.

GITHUB_TOKEN is passed to dotnet test but unused. The C# tests use Testcontainers directly and do not appear to need it. Minor, but cleaner to remove.

Java Changes

The @SuppressWarnings("unchecked") moves are correct - the annotation was misplaced on parseArrayFromString (which has no unchecked casts) and belongs on serializeAsText and serializeArrayToString where the (Map<String, ?>) map casts actually occur. Good cleanup.

The Map<?, ?> map with explicit cast is technically correct here, though note that if a Map with non-String keys reaches this path, the cast will fail at runtime. This is a pre-existing risk, not introduced by this PR.

What Is Good

• The fixture structure mirrors e2e-python well and is easy to follow.
• All new files have correct Apache 2.0 license headers.
• GitHub Actions steps use pinned commit hashes (good security practice).
• The PostgresTypeTest additions for OID 25 (text format, binary format, isArrayType, size) are targeted and correct.
• .gitignore is appropriate for .NET build artifacts.

Summary

The two protocol workarounds are the most important feedback - they reveal real server-side bugs that will affect production Npgsql users and should become follow-up issues. Adding the NuGet cache and artifact upload steps are low-effort improvements worth doing before merge. Otherwise the PR is well-structured and the fix is real and valuable.

Code Review - feat(e2e-csharp): C# E2E Tests + TEXT OID 25 Fix

Overview

This PR does two things:

1. Fixes PostgresType to properly handle TEXT OID 25 (already present in the enum; this PR refactors the @SuppressWarnings placement and Map<?, ?> generics, and adds explicit unit tests confirming coverage)
2. Adds a new e2e-csharp/ module with 7 xUnit tests exercising the PostgreSQL wire protocol via Npgsql 10 and Testcontainers, with a matching CI job

PostgresType.java Changes

Positive:

• The Map → Map<?, ?> change is correct: using the wildcard type avoids the raw-type warning while the explicit cast (Map<String, ?>) is safe in this serialization context.
• Moving @SuppressWarnings("unchecked") from parseArrayFromString (which doesn't actually do an unchecked cast) to serializeAsText and serializeArrayToString (which do) is more precise.
• Removing the unused DateUtils import is good housekeeping.

Concern - @SuppressWarnings scope:
The annotation is placed at method level, suppressing all unchecked warnings in the method. Since the cast (Map<String, ?>) is unavoidable at runtime (the map key type cannot be verified), this is acceptable - but consider whether Map<String, Object> could be used instead, which would allow calling new JSONObject(map) directly without a cast, depending on how JSONObject is constructed.

C# E2E Tests

Positive:

• Using Server Compatibility Mode=NoTypeLoading and No Reset On Close=true in the connection string are critical for ArcadeDB compatibility and are correctly applied.
• The fixture properly initialises the database and document type over HTTP before establishing the Postgres connection.
• The DisposeAsync null checks (if (DataSource is not null)) are safe but redundant given the field is initialised as null! - minor style point.

Issues:

1. ContainerBuilder(image) constructor - verify this compiles

_container = new ContainerBuilder(image)   // ← is this a valid overload in 4.11.0?
    .WithPortBinding(2480, true)
    ...

The standard Testcontainers.NET API is:

_container = new ContainerBuilder()
    .WithImage(image)
    ...

If ContainerBuilder does not have a single-string constructor in 4.11.0, dotnet build will fail. Please confirm with dotnet build output or update to .WithImage(image).

2. Silent exception swallow in ParameterizedInsert

catch (NpgsqlException)
{
    // ArcadeDB sends a RowDescription after INSERT which Npgsql rejects...
}

Catching the base NpgsqlException silently masks any other error that could occur during the INSERT (e.g. schema mismatch, OOM, connection drop). At minimum, catch a more specific exception or assert on the exception message:

catch (NpgsqlException ex) when (ex.Message.Contains("RowDescription"))
{
    // known protocol mismatch - ArcadeDB sends RowDescription after INSERT
}

Better still, this documents a server-side bug (ArcadeDB should not send RowDescription after a DML). That bug should be tracked and fixed rather than permanently worked around in tests.

3. Manual BEGIN/COMMIT in Transaction test

// ArcadeDB only accepts bare BEGIN; Npgsql's BeginTransactionAsync sends
// BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED which ArcadeDB rejects.

This is another server-side protocol incompatibility. Manually sending raw SQL strings means the test is fragile and bypasses Npgsql's connection-state tracking (Npgsql may not recognise the connection as being in a transaction). Consider opening a separate issue to track supporting BEGIN TRANSACTION ISOLATION LEVEL ... in the PostgreSQL wire handler, similar to how the text-OID bug was fixed here.

4. Test data isolation

Tests insert fixed IDs (ci1, ci2, ps1, mp1, pi1, tx1) into a shared NpgsqlTest type. If a test run is retried, a duplicate-key error could occur on the INSERT and the subsequent SELECT would still pass (masking a test that didn't actually insert). Using unique IDs per run (e.g. Guid.NewGuid().ToString("N")[..8]) would make tests idempotent.

5. No test result upload in CI

The job runs dotnet test --logger "trx;LogFileName=test-results.trx" but never uploads the results:

- name: Upload test results
  if: always()
  uses: actions/upload-artifact@...
  with:
    name: csharp-e2e-test-results
    path: e2e-csharp/ArcadeDB.E2ETests/TestResults/*.trx

The existing Python and JS jobs follow similar patterns; this step is useful for diagnosing CI failures.

6. GITHUB_TOKEN env var

The C# job sets GITHUB_TOKEN (copied from Python/JS jobs), but the C# tests don't appear to use it. If it is genuinely not needed, removing it reduces the attack surface.

7. Missing README

e2e-python/ includes a README.md. The new e2e-csharp/ module has run-tests.sh with a usage check, but no README. Adding a brief README (prerequisites, how to run locally, what the tests cover) would be consistent with the existing module.

Unit Tests (PostgresTypeTest.java)

• textTypeCodeIs25, deserializeTextOid25AsString, deserializeBinaryOid25AsString are all clear and add explicit coverage for the bug fix.
• The additions to isArrayTypeFalse and typeSizesAreCorrect are correct (TEXT.isArrayType() == false, TEXT.size == -1).

Summary

The server-side Java changes are low-risk and correct. The C# test infrastructure is well-structured but has a few issues worth addressing before merge: the ContainerBuilder(image) API needs verification, the silent exception catch in ParameterizedInsert should be tightened, and the documented transaction limitation should be tracked as a separate issue rather than just commented around. The test data isolation and missing CI artifact upload are minor but worth fixing.

Code Review

Overview

This PR adds C# end-to-end tests for ArcadeDB's PostgreSQL wire protocol using Npgsql 10 and Testcontainers, and fixes a real bug where Npgsql 10 sends string parameters as TEXT (OID 25) instead of VARCHAR (OID 1043), causing "Type with code 25 not supported" on every parameterized query.

Java Fix (PostgresType.java) - Correct and Well-Targeted

The core fix is solid. Adding TEXT(25, String.class, -1, value -> value) to the enum is the right approach and mirrors how VARCHAR is defined. The deserialize switch already handles VARCHAR, TEXT, BPCHAR as a group (line 493 in the current file), so the new entry slots in cleanly.

The @SuppressWarnings("unchecked") relocation from parseArrayFromString to serializeAsText and serializeArrayToString is actually more correct - the casts happen in the serialize methods, not in the parse method. Minor improvement there.

One observation: the unchecked cast (Map<String, ?>) map in serializeAsText and serializeArrayToString could still cause a ClassCastException at runtime if a Map with non-String keys is passed. The code suppresses the warning but the underlying risk remains. It was present before this PR but it's worth noting since the annotation is being moved.

Unit Tests (PostgresTypeTest.java) - Good Coverage

The three new tests (deserializeTextOid25AsString, deserializeBinaryOid25AsString, textTypeCodeIs25) directly target the bug fix and are clear. The additions to isArrayTypeFalse and typeSizesAreCorrect are correct completions of those existing parameterized checks.

C# E2E Module - Functional but Has Issues

1. ContainerBuilder(image) API

_container = new ContainerBuilder(image)
    .WithPortBinding(2480, true)
    ...

The standard Testcontainers.NET API is:

_container = new ContainerBuilder()
    .WithImage(image)
    .WithPortBinding(2480, true)
    ...

Passing a string directly to the ContainerBuilder constructor is not part of the documented Testcontainers.NET API - the constructor accepts a ContainerConfiguration, not a string image name. This may cause a compilation error in CI. Please verify with dotnet build and switch to .WithImage(image) if needed.

2. Silent exception swallowing in ParameterizedInsert

catch (NpgsqlException)
{
    // ArcadeDB sends a RowDescription after INSERT...
}

Catching the bare NpgsqlException masks any other failure (schema error, connection drop, OOM, unexpected server error). The broader concern is that this documents a server-side protocol violation - ArcadeDB should not send RowDescription after a DML statement. That should be tracked and fixed in the PostgreSQL wire handler rather than permanently worked around in the test suite. At minimum, narrow the catch:

catch (NpgsqlException ex) when (ex.Message.Contains("RowDescription") || ex.Message.Contains("unexpected message"))
{
    // known: ArcadeDB sends RowDescription after INSERT (protocol bug to fix)
}

3. Manual BEGIN/COMMIT bypasses Npgsql connection state

The comment correctly explains why BeginTransactionAsync() cannot be used, but the workaround (sending raw BEGIN/COMMIT strings) means Npgsql's internal connection state machine does not know the connection is in a transaction. This is fragile and the underlying issue (BEGIN TRANSACTION ISOLATION LEVEL READ COMMITTED not being accepted) should be tracked as a separate issue to fix in the wire handler.

4. Test data isolation - fixed IDs can cause flakes on retry

All tests insert rows with hard-coded IDs (ci1, ci2, ps1, mp1, pi1, tx1) into a shared NpgsqlTest type. On a CI retry the INSERT will fail with a duplicate-key or unique-constraint violation, but the subsequent SELECT may still return a row from the previous run, causing tests to appear green despite not having run correctly. Use per-run unique IDs:

var id = Guid.NewGuid().ToString("N")[..8];

5. No test result artifact upload in CI

The job produces a .trx results file but never uploads it, making CI failures harder to diagnose:

- name: Upload test results
  if: always()
  uses: actions/upload-artifact@v4
  with:
    name: csharp-e2e-test-results
    path: e2e-csharp/ArcadeDB.E2ETests/TestResults/*.trx

6. GITHUB_TOKEN in env block

The C# tests don't appear to use GITHUB_TOKEN (unlike the Python/JS jobs which may use it for image pulling or rate-limit avoidance). If it is not needed, removing it reduces the credential exposure surface.

7. No timeout on the test step

Long-running containers could cause the CI job to hang indefinitely. Consider adding timeout-minutes to either the step or the job:

csharp-e2e-tests:
  runs-on: ubuntu-latest
  timeout-minutes: 20

Summary

The Java side is ready to merge. The C# module needs the ContainerBuilder API call verified (potential build failure), and the test isolation + exception handling issues addressed before this is reliable in CI.

Code Review

Overview

This PR adds a new C# end-to-end test suite (e2e-csharp/) that validates ArcadeDB's PostgreSQL wire protocol via Npgsql 10 and Testcontainers, plus minor Java cleanup in PostgresType.java (moving @SuppressWarnings annotations and fixing raw type warnings).

Java Changes (PostgresType.java)

Positive:

• Moving @SuppressWarnings("unchecked") from parseArrayFromString (where no unchecked cast actually occurred) to serializeAsText and serializeArrayToString (where the Map cast does occur) is more precise and correct.
• Replacing the raw Map map with Map<?, ?> map and an explicit (Map<String, ?>) cast improves type safety at compile time.

Minor concern:
The cast (Map<String, ?>) map is still an unchecked runtime cast - if the map keys happen to be non-String, JSONObject(Map<String, ?>) could blow up. This is probably fine in practice (ArcadeDB's internal maps use String keys), but worth noting.

C# E2E Tests

Positive:

• The fixture uses a proper IAsyncLifetime lifecycle with clean container teardown.
• Server Compatibility Mode=NoTypeLoading is the correct Npgsql setting to bypass PostgreSQL catalog queries that ArcadeDB cannot answer.
• Using ARCADEDB_DOCKER_IMAGE env var to override the image (same pattern as JS/Python e2e suites) is consistent.
• The 7 tests provide good breadth of coverage: basic connection, unparameterized query, parameterized select/insert, multi-parameter, TEXT OID 25 specifically, and transaction.

Issues:

1. ParameterizedInsert catches all NpgsqlException silently - this is too broad. A network failure or unrelated protocol error would be swallowed, causing a false-positive test pass. Consider asserting on the exception message or type, or at minimum logging it:

   catch (NpgsqlException ex) when (ex.Message.Contains("RowDescription"))
   {
       // known ArcadeDB protocol deviation: RowDescription sent after INSERT
   }

   Better yet, this known protocol deviation should have a tracking issue reference in the comment.

2. Transaction test bypasses Npgsql's connection state machine - manually sending BEGIN/COMMIT as raw SQL leaves the Npgsql NpgsqlConnection object unaware of the transaction. If the INSERT fails, the connection may be in an inconsistent state (no ROLLBACK issued), potentially poisoning the connection pool for subsequent tests. Consider wrapping in a try/finally with a ROLLBACK or calling conn.ResetAsync() on failure.

3. Hardcoded IDs with shared document type - tests share the NpgsqlTest type and use hardcoded IDs (ps1, mp1, pi1, etc.). If ExecuteNonQueryAsync in ParameterizedInsert throws and the row was actually committed (as the comment says it is), a second test run within the same fixture lifetime will try to insert a duplicate pi1. xUnit IAsyncLifetime creates a new fixture per test class instance, but if the container is shared across runs in CI (e.g., a flaky retry), this could cause spurious failures. Using unique IDs (e.g., Guid.NewGuid().ToString()) would make tests idempotent.

4. SimpleQuery assumes non-empty schema - Assert.True(reader.HasRows) relies on schema:types returning rows. This should always be true since NpgsqlTest was created in InitializeAsync, but if fixture setup fails partially, this test would give a misleading failure.

CI Workflow (mvn-test.yml)

Positive:

• Pinned action SHAs with version comments - consistent with the rest of the file.
• Correctly depends on build-and-package and restores the Docker image from cache.

Issues:

1. No test result upload - every other job in this workflow uploads test results as an artifact. The C# job generates a .trx file but never uploads it. Add an upload step (even if just as a raw artifact):

   - name: Upload test results
     if: always()
     uses: actions/upload-artifact@...
     with:
       name: csharp-test-results
       path: e2e-csharp/ArcadeDB.E2ETests/TestResults/

2. No test report step - other jobs use a test reporter action to surface results inline on PRs. The TRX format can be converted, or an xUnit XML logger can be added (--logger "xunit;LogFileName=results.xml") for compatibility with the existing reporter action.

3. dotnet restore is missing - dotnet test implicitly restores packages, but explicitly calling dotnet restore before dotnet test makes CI failures easier to diagnose (network vs. test failures).

Missing

• No README.md for e2e-csharp/ (the Python module has one; having at least a brief usage note would be consistent).
• The known ParameterizedInsert protocol bug (ArcadeDB sending RowDescription after INSERT) is worked around in the test but deserves a tracked issue so it isn't forgotten.

Summary

The C# e2e suite is a solid addition and the TEXT OID 25 tests clearly document the fix. The main actionable items before merging:

• Narrow the NpgsqlException catch in ParameterizedInsert
• Add a test result upload step to CI
• Consider using unique IDs to make tests idempotent across retries

🤖 Generated with Claude Code

Code Review - PR #4038: C# E2E Tests via Npgsql + TEXT OID 25 Fix

Overview

This PR does two things: (1) fixes a real bug where Npgsql 10 sends string parameters as TEXT OID 25, which ArcadeDB's PostgreSQL wire protocol module previously did not recognize, and (2) adds a C# e2e test suite (7 xUnit tests) to validate the fix and the PostgreSQL wire protocol more broadly. The structure mirrors the existing e2e-python and e2e-js modules, which is a good consistency call.

Java Backend (PostgresType.java / PostgresTypeTest.java)

Positives:

• The TEXT(25, String.class, -1, value -> value) entry in the enum correctly handles OID 25 in both text and binary format deserialization.
• The @SuppressWarnings("unchecked") relocation to the narrower scope (serializeAsText, serializeArrayToString) is correct - the annotation is now closer to the actual unchecked cast.
• Changing Map map to Map<?, ?> map before casting is a cleaner approach to suppress the raw-type warning. That said, the cast (Map<String, ?>) map is still unchecked at runtime - if a caller passes a map with non-String keys, JSONObject's constructor would fail. This is probably fine in practice since ArcadeDB internally produces Map<String, Object> results, but worth a comment.
• Removing the unused DateUtils import and the @SuppressWarnings from parseArrayFromString (which didn't actually need it since the generic type parameter already suppresses the relevant warning) are both clean-up improvements.

Minor concern: Removing @SuppressWarnings("unchecked") from parseArrayFromString may surface an unchecked warning at compile time in some Java compiler configurations because of the ArrayList<T> construction inside a static generic method. It's worth verifying mvn clean install -DskipTests passes without -Xlint:unchecked warnings.

New tests - good:

void deserializeTextOid25AsString()  // text format
void deserializeBinaryOid25AsString() // binary format
void textTypeCodeIs25()               // sanity constant check

These are solid regression tests for the bug fix. Adding TEXT to isArrayTypeFalse and typeSizesAreCorrect is also good completeness.

C# E2E Tests (e2e-csharp/)

ArcadeDbFixture.cs

• ContainerBuilder(image) - the standard Testcontainers.NET API is new ContainerBuilder().WithImage(image). Passing the image string directly to the constructor is non-standard. If this is a valid overload in 4.11.0, fine, but please verify this compiles on a clean machine without IDE help.
• The HTTP client setup for database/type creation uses StringContent with raw JSON strings. This is fragile if the database name or type name ever contain special characters. For a test fixture with fixed values this is acceptable, but consider System.Text.Json.JsonSerializer for robustness.
• The NpgsqlDataSource connection string includes No Reset On Close=true - this suppresses the RESET ALL / DEALLOCATE ALL that Npgsql normally sends on connection return to pool. This works around another ArcadeDB protocol deviation silently. A short comment explaining why would help future readers.

PostgresE2ETests.cs

• ParameterizedInsert test - swallowing the exception in the catch block is the most concerning part of this PR:

  catch (NpgsqlException ex) when (string.IsNullOrEmpty(ex.SqlState))
  {
      // ArcadeDB sends a RowDescription after INSERT which Npgsql rejects ...
  }

  This hides a known protocol deviation (ArcadeDB sending RowDescription after INSERT) rather than surfacing it. The test passes, but it does so by catching and ignoring a protocol error, which means the test passes regardless of whether the insert actually worked. The subsequent SELECT does validate the data is there, so the end-to-end check is still valid - but the silent catch makes it hard to notice if the behavior changes. At minimum, the comment should be expanded; consider using Assert.Skip() with a tracking issue reference instead.

• Transaction test - The workaround of sending raw BEGIN/COMMIT strings instead of using Npgsql's BeginTransactionAsync is well-commented. This is acceptable for documenting ArcadeDB's current limitation, but consider adding a // TODO: use conn.BeginTransactionAsync() once ArcadeDB supports full BEGIN syntax so the debt is trackable.

• Test isolation - Tests use hardcoded IDs (ci1, ps1, pi1, mp1, tob1, tx1) in the shared NpgsqlTest type. They are in the same [Collection("ArcadeDB")] which serializes them in xUnit, so parallel execution within the collection is not a risk. However, the hardcoded IDs mean re-running a test suite without a fresh container could produce unexpected results (e.g., duplicate key conflicts). Since the fixture creates a fresh container per run, this is fine in CI - just worth noting.

• No negative/error path tests - All 7 tests are happy-path. A test verifying that an unsupported type or malformed query returns a meaningful error rather than a crash would strengthen confidence in protocol robustness.

CI (mvn-test.yml)

• The job structure mirrors js-e2e-tests and python-e2e-tests, which is consistent. Good.
• No test result artifact upload - Other e2e jobs may also skip this, but uploading the .trx file as a GitHub Actions artifact (actions/upload-artifact) would make debugging CI failures easier.
• No timeout - If the ArcadeDB container fails to become healthy, the job will hang until GitHub's 6-hour default. Consider adding timeout-minutes: 15 to the job.
• dotnet-version: "10.0" targeting .NET 10 is fine for now, but .NET 10 is a non-LTS release. Consider whether net9.0 (LTS) or a floating "10.x" would be better for CI stability.

Summary

The bug fix itself is sound and the test additions are valuable. The main asks before merge are: (1) verify ContainerBuilder(image) compiles cleanly, (2) improve the exception-swallowing in ParameterizedInsert, and (3) add a timeout-minutes to the CI job.

🤖 Generated with Claude Code