Conversation
459c5cc to
cb416fb
Compare
| { | ||
| Assert.Throws<TypeDBDriverException>(() => TypeqlUpdate(updateQueryStatements)); | ||
| if (_queryOptions != null) | ||
| return Tx.Query(queryText, _queryOptions); |
There was a problem hiding this comment.
Huh that's smart! we actually don't need to write 'read query' or 'write query' at all since we've already explicitly written the txn type to use!
There was a problem hiding this comment.
This is worth a look
There was a problem hiding this comment.
this is also worth a look
There was a problem hiding this comment.
Tell Claude to reimplement the integration tests existing in Python (since it has its own datetime impl, too, it is important). Specifically, test_values.py. This will ensure that these classes are correct.
d734000 to
f81d1ec
Compare
csharp/Test/rules.bzl
Outdated
| ) | ||
|
|
||
|
|
||
| def csharp_xunit_integration_test(name, srcs, deps, target_frameworks, targeting_packs, **kwargs): |
There was a problem hiding this comment.
TBD: do we need this?
There was a problem hiding this comment.
Idk, this requires context retrieval. If the tests worked fine before, and we haven't updated the rules, it should still be fine.
There was a problem hiding this comment.
Ok i managed to revert to the old system - keeping it in place
3be51da to
e15413b
Compare
flyingsilverfin
left a comment
There was a problem hiding this comment.
Ready for review!
There was a problem hiding this comment.
@dmitrii-ubskii @farost worth looking at closely
There was a problem hiding this comment.
@farost worth a skim but otherwise maybe test coverage is good enough? Up to you
There was a problem hiding this comment.
The %noexception directives should be identical to the java file. Haven't compared them directly, the tests must catch everything, because the memory blows up if something is wrong here (when you actually use these methods).
There was a problem hiding this comment.
New stuff - pretty much directly copies Java's
| filegroup( | ||
| name = "steps-cloud", | ||
| srcs = ["ConnectionStepsBase.cs", "ConnectionStepsCloud.cs"], | ||
| name = "steps-cluster", |
There was a problem hiding this comment.
@farost i had claude stub out the expected future use for you
|
|
||
| Driver!.Close(); | ||
| public static ITypeDBTransaction OpenTransaction( |
There was a problem hiding this comment.
@farost this has changed to be inlined here so you'll want to outline it back into the subclasses again when you do cluster
| @@ -47,6 +50,45 @@ def csharp_behaviour_test( | |||
| ) | |||
|
|
|||
|
|
|||
| def csharp_behaviour_test_all( | |||
| deps = deps, | ||
| target_frameworks = target_frameworks, | ||
| targeting_packs = targeting_packs, | ||
| add_certificates = True, # Cluster needs TLS certificates |
There was a problem hiding this comment.
Not sure if the comment is right @farost for you to pick up in the future!
There was a problem hiding this comment.
It is right, but the impl looks different from Java and the other languages. I'll tackle it on the cluster side
There was a problem hiding this comment.
@dmitrii-ubskii pls review this one's changes, i'm out of context here now
There was a problem hiding this comment.
Would be good to see the context of when it was done and why, how to trigger an issue
There was a problem hiding this comment.
Ok this was able to be reverted, no problem!
| [When(@"get answers of typeql insert")] | ||
| [Then(@"get answers of typeql insert")] | ||
| public void GetAnswersOfTypeqlInsert(DocString insertQueryStatements) | ||
| [Given(@"get answers of typeql schema query; fails")] |
There was a problem hiding this comment.
Eh, sad there are no smart params to recreate MayError
There was a problem hiding this comment.
I had a go at doing MayError with regex, but it didn't work and ended up with 0 savings, so we'll keep this approach
There was a problem hiding this comment.
Tell Claude to reimplement the integration tests existing in Python (since it has its own datetime impl, too, it is important). Specifically, test_values.py. This will ensure that these classes are correct.
| deps = deps, | ||
| target_frameworks = target_frameworks, | ||
| targeting_packs = targeting_packs, | ||
| add_certificates = True, # Cluster needs TLS certificates |
There was a problem hiding this comment.
It is right, but the impl looks different from Java and the other languages. I'll tackle it on the cluster side
There was a problem hiding this comment.
Would be good to see the context of when it was done and why, how to trigger an issue
| try | ||
| { | ||
| Pinvoke.typedb_driver.transaction_force_close(NativeObject); | ||
| // Call transaction_close and wait for it to complete. |
There was a problem hiding this comment.
Kinda makes sense. Wdyt? Does it mean that we always need to await transaction closings? @dmitrii-ubskii
24b7382 to
e30d102
Compare
Upgrade the C# driver to TypeDB 3.0 API with unified entry point and sessionless transactions: - Add TypeDB.Driver() unified entry point replacing Drivers.CoreDriver() - Add IDriver interface with Transaction() methods (no Session layer) - Add Credentials, DriverOptions, TransactionOptions, QueryOptions classes - Add ITypeDBTransaction.Query() for direct query execution - Update TransactionType enum with Schema type for 3.0 - Remove Session layer (ITypeDBSession, TypeDBSession) - Remove managers (ConceptManager, LogicManager, QueryManager) - Remove legacy answer types (ConceptMap, ConceptMapGroup, ValueGroup) - Update DatabaseManager and UserManager for 3.0 native API - Update test step definitions for 3.0 connection and transaction flow 8 of 14 database behavior tests pass (parallel test has native crash). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Key changes: - TypeDBDriver: Store Credentials and DriverOptions references to prevent GC - TypeDBTransaction: Store TransactionOptions, resolve close/onClose promises - ConnectionStepsBase: Simplified cleanup to reuse existing driver - TransactionSteps: Updated BUILD to remove obsolete dependencies Note: 2/43 transaction tests pass. Tests crash on 3rd driver creation - likely a native library issue that needs further investigation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changes: - Add GC.KeepAlive() calls to prevent GC from collecting objects during native calls - Add guards to IsOpen() and Close() for disposed objects - Add sleep between test scenarios (matching Java driver workaround) - Match Java's pattern for driver and transaction close/cleanup Known issue: Native crash on 3rd consecutive driver creation - Transaction tests: 2/43 pass before crash - Database tests: 8/14 pass before crash - The issue appears to be C# GC finalization timing differing from Java - Rust driver's async cleanup may race with C# GC finalization - Further investigation needed at the Rust FFI layer 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update BDD test step definitions and BUILD files to support TypeDB 3.0 unified query API patterns. All test-core targets now build successfully. Key changes: - Rewrite QuerySteps.cs for unified tx.Query() API - Add Answer types (IQueryAnswer, IConceptRow, IConceptDocumentIterator) - Update Concept API with value type checks and accessors - Remove obsolete Session step references from BUILD files - Add connection reset database step for test cleanup - Remove Get and RuleValidation tests (features removed in 3.0) - Add IncludeQueryStructure to QueryOptions Tests build and run but crash after 1-2 scenarios due to known SWIG native memory management issue (documented in ConnectionStepsBase.cs). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Changes: - Add 'uniquely identify answer concepts' step with matcher-based approach supporting label, key, attr, value, and none identifiers - Add 'transaction commits; fails with message' step for error checking - Add 'Then' attribute to transaction opening steps - Fix 'connection delete database; fails' step to handle both Get and Delete failures - Add DRIVER_LOCK global mutex in C layer to prevent GC finalization race conditions - Add transaction_close_sync for synchronous close with callback completion - Update SWIG bindings for C# director callbacks Test results: - Insert tests: 128 passed, 6 skipped - Define tests: 445/446 passed - Database/Transaction tests have pre-existing parameterization issues 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The global mutex was originally added to serialize connection-related operations for C# GC finalizer thread safety. However, this approach bottlenecks all language bindings (Java, Python) that share the C layer. Removing the lock from the C layer because: 1. Tests pass without it, showing no race conditions in practice 2. If C#-specific thread safety is needed, it should be handled in the C# layer only, not penalizing other language bindings 3. The Rust driver internally handles thread safety appropriately Memory management: The key difference is that C# must manually keep references alive due to its concurrent GC/finalizer model, while Java and Python can rely on their runtime's built-in protections. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Factory CI (automation.yml): - Add bazel build //csharp/... to the build job - Add test-csharp-behaviour-community job for BDD tests CircleCI (config.yml): - Enable deploy-snapshot-dotnet-any job in snapshot workflow - Enable deploy-release-dotnet-any job in release workflow - Enable Windows C# deployment scripts (snapshot and release) - Add deploy-snapshot-dotnet-any as requirement for test jobs - Add deploy-release-dotnet-any as requirement for deploy-github 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove obsolete 2.x documentation (Session, Logic, IConceptMap, etc.) and regenerate documentation for the 3.x API which removes Sessions and introduces IQueryAnswer, IConceptRow, and IConceptDocumentIterator. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The previous implementation incorrectly boxed Arcs and type-punned pointers, causing FFI issues. This restores the correct pattern using Arc::into_raw/Arc::from_raw directly. Changes: - memory.rs: Remove Box<Arc<T>> indirection, use Arc::into_raw directly - database.rs: Revert to using borrow() and take_arc() - database_manager.rs: Restore try_release_arc and iterator_arc_next - iterator.rs: Remove iterator_boxed_arc_next, restore iterator_arc_next - error.rs: Make record_error private again - TypeDBDatabase.cs: Simplify Delete() to match original API Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add datetime-tz and duration value type handling in QuerySteps - Add fallback comparison using native string representation - Fix decimal parsing to strip 'dec' suffix - Improve datetime parsing for nanosecond precision - Fix test isolation by cleaning up leftover databases in CleanInCaseOfPreviousFail - Add clear error message for <type> placeholder limitation in Gherkin - Update test status documentation Test results: Database, Define, Insert, Delete fully passing. Transaction 39/43 (4 placeholder limitation), Match 120/127 (missing steps + native issues) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove debug logging from SWIG interface while keeping the thread-safe callback handling. Simplifies the code back to its original structure. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add custom %typemap(csbody) for QueryAnswer that generates IntoRows() and IntoDocuments() instance methods which properly handle ownership transfer by setting swigCMemOwn = false before calling the native functions. This mirrors the approach used in the Java SWIG bindings. Also add missing 3.0 API iterators (ConceptRow, StringAndOptValue) to the C# SWIG configuration. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add functor-encoder and Analyze to behaviour_tests_deps so all behaviour tests automatically have them. Remove explicit deps from individual test BUILD files. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Align C# driver naming with Java/Python conventions: - Rename Thing -> Instance (IThing -> IInstance, Thing.cs -> Instance.cs) - Rename IEntity, IRelation, IAttribute to extend IInstance instead of IThing - Remove ThingType intermediate class - EntityType, RelationType, AttributeType now extend Type directly - Update IConcept to use AsInstance()/IsInstance() instead of AsThing()/IsThing() - Update examples to use TryGetIID() instead of .IID property Add comprehensive values integration tests: - Rust: tests for Duration, Decimal, Datetime, DatetimeTZ parsing and database round-trips mirroring Python's test_values.py - C#: ValuesTest with unit tests for datetime/duration classes and integration tests for all value types through TypeDB Add nanosecond precision comments to DatetimeTZ constructors to document that the subsecNanos parameter preserves TypeDB's full nanosecond precision. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add active test-csharp-integration job that runs //csharp/Test/Integration/Data:all - Remove old commented-out C# test blocks (test-csharp-integration, test-csharp-behaviour-community, test-csharp-behaviour-cluster) that referenced non-existent paths - Add TODO comment for Node.js/C++ tests that need 3.x updates Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Introduces MayError class that reduces BDD step definition duplication by allowing one step definition to handle both success and failure cases via regex optional capture groups. - MayError.cs: Parses error mode strings like "; fails", "; parsing fails", or "; fails with a message containing: \"...\"" - QuerySteps.cs: Refactored query execution steps using MayError pattern, also fixed remaining Thing→Instance API changes - ConceptSteps.cs: Fixed remaining Thing→Instance API changes - docs_structure.bzl: Removed obsolete IThingType/IThing doc references Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The regex alternative pattern with Xunit.Gherkin.Quick wasn't working correctly. Reverted to separate step definitions while keeping MayError helper for cleaner implementation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The MayError regex alternative pattern didn't work with Xunit.Gherkin.Quick, so the helper provided no actual benefit over direct Assert calls. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Regenerated docs reflecting Thing→Instance refactoring and analyze API updates. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Use built-in csharp_nunit_test instead of custom csharp_xunit_integration_test rule, simplifying build configuration and removing explicit xUnit dependencies. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update removed_lines filter from Xunit to NUnit after test framework change - Add csharp to DOCS_DIRS in build-docs CI job Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Align C# SWIG configuration with Java to disable exception checking for functions that never fail, reducing P/Invoke overhead. Added directives for: - Transaction options parallel getters/setters - Query answer converters (into_rows, into_documents) - Concept row accessors (column_names, concepts, conjunctions) - Concept type checks (is_entity, is_relation, etc.) - Concept casts (as_entity, as_relation, etc.) - Concept value type checks (is_boolean, is_integer, etc.) - Concept metadata accessors (get_label, try_get_*, equals, to_string) - Instance type getters (entity/relation/attribute_get_type) - Struct field accessors (StringPair, DatetimeAndTimeZone, etc.) - Additional destructors (DriverOptions, Value types, Promises) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Remove DebugSteps.cs which contained temporary debugging steps for investigating multi-driver scenarios. Keep the debug test infrastructure (BUILD, DebugTest.cs, debug.feature) for ad-hoc debugging. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Match Java's pattern of using the API constant directly instead of a test-specific constant. This removes the need for the ServerAddress replacement in test_to_example. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Include Datetime.cs, DatetimeTZ.cs, and Duration.cs in docs_source_files and add corresponding mappings to docs_structure.bzl. These are C#-specific types needed because System.DateTime lacks nanosecond precision. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The timeout-based polling and explicit gRPC worker joining added to fix C# GC race conditions are no longer needed. Other fixes in the C# layer (proper cleanup ordering, synchronous transaction close) have resolved the underlying issues. This reverts to the simpler original implementation: - Callback handler uses blocking recv() instead of timeout polling - gRPC worker thread JoinHandle is not stored/joined explicitly - Drop order returns to: drop sink, then join callback handler Validated by running C# behavior tests (Database, Transaction, User, Query) - all pass without crashes. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add tests that verify OnClose callbacks are invoked correctly: - OnCloseCallbackIsInvokedOnExplicitClose: explicit close invokes callback - OnCloseCallbackIsInvokedOnGarbageCollection: GC finalization invokes callback - MultipleOnCloseCallbacksAreAllInvoked: multiple callbacks all fire The GC finalization test validates that transaction_close_sync is necessary in the SWIG destructor - without it, the test segfaults due to callbacks being invoked after SWIG directors are finalized. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The documentation incorrectly stated "Closes the transaction" with example "transaction.close()" for the is_open() method. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
81021b0 to
2a5a28f
Compare
…nsaction_drop_sync Simplify to 2 transaction close variants: - transaction_drop_sync: close + wait for callbacks + free memory (destructor) - transaction_close: returns promise, doesn't free (explicit close) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dff8b9a to
b6a8d7a
Compare
- Remove csharp-driver-3.0-plan.md (working memory, not needed in repo) - Update TODO comment to remove C# from list of drivers needing 3.x update Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add %promise macro for QueryAnswerPromise in C# SWIG - Use promise.Resolve() instead of manual Released() + raw P/Invoke - SWIG Resolve() handles ownership transfer automatically Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Change Query() and Analyze() to return Promise<T> instead of resolving synchronously - Update all call sites (54 locations) to use .Resolve() - Add query string validation matching Java implementation - Move callback cleanup to finally block in Close() for proper error handling Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
0b55321 to
b16c28c
Compare
The C# source now uses proper XML doc comments (<summary>, <exception>, <example>) instead of Javadoc-style comments. Doxygen converts these to HTML with <dl> and <div> elements that weren't being handled correctly, causing weird whitespace and raw exception text in the generated docs. Changes: - Add stripDocSections() to remove exception, return, and section blocks - Extract code examples from <div class="fragment"> (new Doxygen format) - Regenerate all C# documentation with the fix Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Java's requireNonNull only checks for null, allowing empty strings to pass to the server which returns "syntax error". The C# driver was using NonEmptyString which rejected empty strings locally with a different error message, causing BDD test failures. Changes: - Add NonNull validator method - Add NON_NULL_VALUE_REQUIRED error message - Change Query() and Analyze() to use NonNull instead of NonEmptyString Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
farost
left a comment
There was a problem hiding this comment.
Approved with final comments regarding the naming and some excessive parts
| // ============================================================================ | ||
|
|
||
| [Test] | ||
| public void DatetimeParse() |
There was a problem hiding this comment.
A good test. Would be good to express it through BDDs and ignore in the HTTP drivers, but cover everywhere else. I think I had a small attempt (but then discovered the feature did not work, and we buried it).
I can look into this myself later, or just implement similar integration tests everywhere else. What would you prefer?
There was a problem hiding this comment.
Let's add it to the other drivers. I had already added them as a single test in test_driver to Rust as transaction_callback and in Java it's transaction_on_close and python it's test_on_close_callback. Although those are simpler! Perhaps after merging we can have claude push the same higher resolution tests into each language
Lemme streamling the naming so they're all transaction_on_close_callback as the test or test function suffix
csharp/TypeDB.cs
Outdated
| /// <param name="credentials">The <see cref="Credentials"/> to connect with.</param> | ||
| /// <param name="driverOptions">The <see cref="DriverOptions"/> to connect with.</param> | ||
| /// <returns>An open <see cref="IDriver"/> connection to the TypeDB server.</returns> | ||
| public static IDriver Driver(Credentials credentials, DriverOptions driverOptions) |
There was a problem hiding this comment.
Do you like these overloaded constructors with default values? I guess I'm just used to Rust with more explicit declarations
There was a problem hiding this comment.
you know what i'm just going to eliminate all except the explicit constructor with all arguments - matching Java's. Good catch!
There was a problem hiding this comment.
Oh, I just remembered! Since we declare the drivers in TypeDB.cs, these must be called just Database.cs, Driver.cs, DatabaseManager.cs, etc. That's how we approached it in Java. The rule we came up with was something like:
The entry point must declare it's a part of TypeDB, everything else must not be that specific. If the entry point is the driver itself, it must be called
TypeDBDriver. If the driver is spawned in a namespace or a static object, that namespace or static object must be calledTypeDB, and the driver will not need the prefix. Everything else is resolved by package and namespace names.
There was a problem hiding this comment.
Here's what I've done - i've renamed managers to be without the TypeDB prefix... but i could not for Driver, because that's a namespace name and it gets confused (and i think it's also confusing). So now we have the same as java except that we also have TypeDB.Driver.Connection.TypeDBDriver instead of TypeDB.Driver.Connection.Driver
Usage and product changes
This PR ports the C# driver to the TypeDB 3.0 API, aligning it with the Java and Rust drivers. The driver now uses the simplified 3.0 architecture where sessions are removed and transactions are opened directly from the driver. Query execution returns
IQueryAnswerwith streamingIConceptRowresults instead of the previousIConceptMapmodel.Key API changes include:
ITypeDBDriverrenamed toIDriverwith direct transaction access viaTransaction(database, type, options)IQueryAnswerandIConceptRowtypes replaceIConceptMapfor query resultsIJSONtype for fetch query results with document iterationITypeDBTransaction.Analyze()returningIAnalyzedQuerywith full pipeline introspectionDatetimeTZandDurationtypes for temporal values with nanosecond precisionQueryOptionsandTransactionOptionsreplace the monolithicTypeDBOptionsImplementation
The driver is restructured around the 3.0 API model. The connection layer removes
ITypeDBSession, withTypeDBDrivernow directly managing transactions.TypeDBTransactionis simplified to hold a native transaction handle and expose query methods (Query,Analyze) that return the new answer types.The concept layer is refactored to remove
IConceptManagerand move operations onto concepts themselves. Type hierarchies are simplified—IThingType,IAttributeType, andIRelationTypelose most schema manipulation methods as these move server-side. Value handling is updated with newDatetime,DatetimeTZ, andDurationclasses providing proper temporal type support with IANA timezone handling and nanosecond precision.The answer layer introduces
QueryAnswer,ConceptRow, andJSONtypes wrapping native iterators.ConceptRowIteratorandConceptDocumentIteratorprovide streaming access to results. The analyze API adds a complete wrapper layer incsharp/Api/Analyze/(15 interfaces) andcsharp/Analyze/(14 implementations) exposing pipeline stages, constraints, conjunctions, and type annotations.Native object lifecycle is simplified by removing manual
Disposepatterns and stored references—the Rust FFI layer clones all parameters so C# objects don't need to prevent GC during calls.BDD test infrastructure is updated for 3.0 with new step definitions across connection, query, concept, and user features. Test isolation is fixed to prevent inter-scenario state leaks. A
FunctorEncoderutility supports analyze test assertions. The test helper centralizes value parsing/formatting fordatetimeanddurationtypes. Cluster test infrastructure and endpoints are stubbed out for future implementation.Bazel
BUILDfiles are updated throughout the C# package structure. CI/CD configuration is updated to enable C# driver builds and deployments. AsciiDoc API reference documentation is regenerated for the 3.0 API surface, including new files foranswer types,connection types, and 54 files for theanalyze API.