-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce C# driver without deployment #609
Conversation
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
A quick comment about NamingA typical class's signature is:
C# identifier names coding conventions Exception: we decided to mark public constants LIKE_THIS, because the rules from the Microsoft docs were not clear... Maybe we'll come up with something within this review. Namespaces and directories: After viewing some of the open-source C# projects, we came up with naming everything starting with an Uppercase char and declaring one namespace per library (Api, Common, Concept, Connection, ...). The last rule is broken in Behaviour/Utils, but I'll additionally comment it inside the code. It also means that to run a test like in java: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to clean everything up, it's ready for your review.
%} | ||
|
||
|
||
%typemap(csbase) Error "System.Exception"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example from the SWIG C# doc. This block rethrows native exceptions to C#.
%csconst(1); | ||
|
||
%typemap(cscode) SWIGTYPE %{ | ||
public $csclassname Released() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release memory in a Java-like way.
static NativeExceptionHelper exceptionHelper = new NativeExceptionHelper(); | ||
%} | ||
|
||
%exception %{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is pasted in the generated files after every native call (except of the calls marked %noexception lower, it is the same for Java).
%noexception transaction_commit; | ||
%noexception transaction_rollback; | ||
|
||
%define %promise(Prefix, Type, function_prefix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our Promises. Decided to implement it the same way as in java without architectural refactoring.
TypeQLInsert typeQLQuery = TypeQL.parseQuery(String.join("\n", insertQueryStatements)); | ||
return tx().query().insert(String.join("\n", insertQueryStatements)); | ||
return tx().query().insert(String.join("\n", insertQueryStatements)).collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more correct to try collecting it even if we don't check the value in the end, because we could skip some of the fails. However, it's more like a task for BDD to call for "check", so I'm not really sure if we need to collect on the "Given" step... Ready to roll back and make the same move for C# if you say so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given
doesn't really mean anything in this case and may as well be Step
. The actual step keywords are ignored.
.collect()
could be replaced with .next()
if it weren't for the fact that this "step" is reused as a helper function by other steps. As things stand though, this seems to be a reasonable compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with it as long as you are.
java/test/behaviour/query/language/rulevalidation/RuleValidationTest.java
Outdated
Show resolved
Hide resolved
@@ -103,6 +103,6 @@ generic_step_impl! { | |||
|
|||
#[step(expr = "verify answers are consistent across {int} executions")] | |||
async fn verify_answers_are_consistent_across_executions(_context: &mut Context, _executions: usize) { | |||
// We can't execute previous query again because don't remember the query | |||
// We can't execute previous query again because don't remember the query // TODO: start saving the query and implement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to create a task AND reference this task from the code so these TODOs could be found more easily?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thank you! What do you think about starting referencing github issues inside the code TODOs? For example:
// TODO #612: start saving the query and implement
It would help us cleaning outdated todos and remembering reasons behind each of them on a long distance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partial review
@@ -32,7 +32,7 @@ cucumber_bdd::StepCollection<Context> commonSteps = { | |||
BDD_STEP("wait (\\d+) seconds", { | |||
std::this_thread::sleep_for(std::chrono::seconds(atoi(matches[1].str().c_str()))); | |||
}), | |||
BDD_NOOP("set time-zone is: (.*)"), | |||
BDD_NOOP("set time-zone is: (.*)"), // TODO: Decide if we want to implement (can be changed only for POSIX). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably okay for now.
|
||
|
||
csharp_library( | ||
name = "api", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the target names follow the package names and also be named in UpperCamelCase
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually a good point!
However, it looks like (if we want to follow it...) it's even worse: following the guideline, our targets (which are dlls) should be like Typedb.Driver.***
as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be especially confusing in terms of tests: Typedb.Driver.Behaviour.Connection.Database.Steps
, Typedb.Driver.Behaviour.Connection.Database.TestCore
...
Not sure if we want, although it would be better to make user-facing libs following the convensions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the 'right' convention should be to use our Bazel convention for targets, and set a separate name for any libraries we produce that are 'imported' afterward? ie. in java we can call the package API, but what matters is the package declaration at the top of the file... is that something that happens in C#?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I can't specify a separate name for the library file using the rule ([impl](https://github.com/bazelbuild/rules_dotnet/blob/862c7f5b8bc9d13de064ef0183e30da11034f3a4/dotnet/private/rules/csharp/library.bzl#L4)
). Moreover, these files are generated without namespaces/packages in their names. so its just driver-csharp.dll
, api.dll
, common.dll
for now. So I'm not sure what to follow and if we want to try to rename these files with an additional rule to have TypeDB.Driver.Api
and... Uhm... TypeDB.Driver
?
|
||
csharp_library( | ||
name = "api", | ||
srcs = glob(["*.cs", "*/*.cs", "*/*/*.cs"]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be **/*.cs
, which is recursive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I copied what Java does, and this style is more explicit for the architecture's understanding, I guess. I'd leave it like this, but I'm ready to refactor it if you want me to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK as is!
IType AsType() | ||
{ | ||
throw new TypeDBDriverException( | ||
ConceptError.INVALID_CONCEPT_CASTING, this.GetType().Name, typeof(IType).Name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does that mean for us?
Is typeof(this)
going to be different from this.GetType()
, i.e. IConcept
rather than EntityTypeImpl
?
csharp/Api/Concept/Value/IValue.cs
Outdated
|
||
public class ValueType : NativeObjectWrapper<Pinvoke.ValueType> | ||
{ | ||
public static readonly ValueType OBJECT = new ValueType(typeof(object), false, false, Pinvoke.ValueType.Object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we associate the variants with native ValueType
s here, and turn ValueClass
etc. into get
properties?
* driver.Databases.All; | ||
* </pre> | ||
*/ | ||
IList<IDatabase> All { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In our examples it's usually written as:
IDatabaseManager databases = driver.Databases;
databases.All;
Which is a bit better.
csharp/Api/Answer/IConceptMap.cs
Outdated
* conceptMap.Variables; | ||
* </pre> | ||
*/ | ||
IEnumerable<string> Variables { get; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- If it's a network call, it should probably be a method as well, to make it obvious that it's not free. Ex.:
IDatabaseManager.All
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My previous comment got outdated for some reason, but I'll repeat it: this test was missing in Java!
csharp/Api/Concept/Type/IType.cs
Outdated
*/ | ||
Promise<bool> IsDeleted(ITypeDBTransaction transaction); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⛔️❗️
csharp/Api/Concept/Type/IType.cs
Outdated
*/ | ||
Promise<bool> IsDeleted(ITypeDBTransaction transaction); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check IDEA settings.
csharp/Api/Concept/Value/IValue.cs
Outdated
|
||
public class ValueType : NativeObjectWrapper<Pinvoke.ValueType> | ||
{ | ||
public static readonly ValueType OBJECT = new ValueType(typeof(object), false, false, Pinvoke.ValueType.Object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are Pinvoke.ValueType
s not integers?
csharp/Test/BUILD
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't help but notice there aren't any changes in .factory/automation.yml
, that's something that should be sorted out soonish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
@@ -103,6 +103,6 @@ generic_step_impl! { | |||
|
|||
#[step(expr = "verify answers are consistent across {int} executions")] | |||
async fn verify_answers_are_consistent_across_executions(_context: &mut Context, _executions: usize) { | |||
// We can't execute previous query again because don't remember the query | |||
// We can't execute previous query again because don't remember the query // TODO: start saving the query and implement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
java/test/behaviour/query/language/rulevalidation/RuleValidationTest.java
Outdated
Show resolved
Hide resolved
TypeQLInsert typeQLQuery = TypeQL.parseQuery(String.join("\n", insertQueryStatements)); | ||
return tx().query().insert(String.join("\n", insertQueryStatements)); | ||
return tx().query().insert(String.join("\n", insertQueryStatements)).collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given
doesn't really mean anything in this case and may as well be Step
. The actual step keywords are ignored.
.collect()
could be replaced with .next()
if it weren't for the fact that this "step" is reused as a helper function by other steps. As things stand though, this seems to be a reasonable compromise.
csharp/nuget/BUILD
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this package be moved under "dependencies/nuget", next to "dependencies/maven"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno, maybe! I put it inside csharp as it's a csharp-only thing, but if you want to store such dependencies in a specific place, I'm not against moving it.
{ | ||
public abstract class ErrorMessage | ||
{ | ||
private readonly string _codePrefix; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(duplicate old comment as it was outdated within the refactoring):
readonly vs { get; }:
*If I expect to always get the same result for each call, it's a readonly field.
*If I expect that the result can be different, it is a property with a getter (with no setters).
*If we call a native function under the hood, it's a property with a getter (with no setters).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good already, almost there!
Note: your integration tests are very complete, hopefully they didn't take you too long and were useful during development.
public $csclassname Released() | ||
{ | ||
var cPtr = swigCPtr.Handle; | ||
if (swigCMemOwn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to have a race condition here? Can the memory be released by two threads concurrently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't think about it! I can wrap the body of this method in lock(this) {}
just as the SWIG-generated Dispose
does it just not to worry about it.
protected virtual void Dispose(bool disposing) {
lock(this) {
if (swigCPtr.Handle != global::System.IntPtr.Zero) {
if (swigCMemOwn) {
swigCMemOwn = false;
typedb_driverPINVOKE.delete_Session(swigCPtr);
}
swigCPtr = new global::System.Runtime.InteropServices.HandleRef(null, global::System.IntPtr.Zero);
}
}
}
@dmitrii-ubskii what are you thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock(this) { ... }
in Release()
is sensible iff every place that directly accesses swigCMemOwn
or swigCPtr
(like getCPtr()
or equivalent) also locks on this
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, not every place, just Dispose
. swigRelease
does not, that's why I am unsure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case Release()
twice without a lock doesn't introduce extra races, because those were already present for all native calls.
Illustration for why the lock wouldn't help here: https://dotnetfiddle.net/kO2zJf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, yeah, you're right about the races and locks, looks like it already works this way, I guess...
%promise(String, string, string) | ||
%promise(Bool, bool, bool) | ||
|
||
/* void promises require special handling */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: you can use code blocks (single or triple ticks) to write whatever you want:
TemplatedClass<void>
%} | ||
|
||
%typemap(csdisposing, methodname="Dispose", methodmodifiers="protected", parameters="bool disposing") VoidPromise %{{ | ||
lock(this) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this to prevent race conditions? What's thie method Dispose
versus Released
at the top? They look quite similar!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dispose
is auto generated by SWIG and is a method of freeing unmanaged resources when leaving using
blocks with allocated memory for IDisposable
interface. You see it here as I use %typemap(csbody)
typemap to rewrite the body of the generated class, thus erasing this automatically generated method, so I just return it here.
But yes, they want to prevent race conditions here (with a bit dirty way, tbh).
On the other hand, Released
is a method for manual release of our resources in specific situations, similar to Java Driver.
# 3. Check the fetched packages in the regenerated 'paket.csharp_deps.bzl' file. | ||
# 4. Reference this packages in your BUILD files via '@paket.csharp_deps//{package.name.in.lowercase}'. | ||
|
||
paket install |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means that CSharp build is not hermitic right? We should outline this in the PR description and note the build requirements on the local system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it depends on what we consider as hermetic. You're right that this file is not hermetic (I forgot about it), but it is needed only if you want to run paket2bazel
(read as "run update.sh
"), which is needed only for developers when you update dependencies. The driver and its build afterwards is hermetic and I've already "tested" it while setting up my Linux env: I just needed to install Bazel and fetch the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see - build is hermitic, but updating dependencies is not. Correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
public void SendArraysToNativeSide() | ||
{ | ||
string dbName = "mydb"; | ||
ITypeDBDriver driver = Utils.OpenConnection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: in your PR description, you should also use driver
as the name for a variable holding a driver instance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do, it's inside the using
!
95d1436
to
914ff51
Compare
public $csclassname Released() | ||
{ | ||
var cPtr = swigCPtr.Handle; | ||
if (swigCMemOwn) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock(this) { ... }
in Release()
is sensible iff every place that directly accesses swigCMemOwn
or swigCPtr
(like getCPtr()
or equivalent) also locks on this
.
.factory/automation.yml
Outdated
python3.9 -m pip install -U cffi | ||
export SYNC_DEPENDENCIES_TOKEN=$REPO_GITHUB_TOKEN | ||
bazel run @vaticle_dependencies//tool/sync:dependencies -- --source ${FACTORY_REPO}@${FACTORY_COMMIT} | ||
sudo add-apt-repository ppa:deadsnakes/ppa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #610
sudo add-apt-repository ppa:deadsnakes/ppa | |
sudo add-apt-repository -y ppa:deadsnakes/ppa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, it has been lost within rebases. Sorry for that
c/swig/typedb_driver_csharp.swg
Outdated
arrayPtr[i] = global::System.Runtime.InteropServices.Marshal.StringToCoTaskMemAnsi($csinput[i]); | ||
} | ||
|
||
arrayPtr[i] = global::System.IntPtr.Zero; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really just be arrayPtr[arraySize]
so it's both explicit and we don't have to depend on the for-loop side effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
cpp/lib/common/error_message.cpp
Outdated
@@ -32,7 +32,7 @@ namespace TypeDB { | |||
|
|||
namespace DriverError { | |||
|
|||
#define ERR_DRIVER(ID, MSG) ERRMSG("CCL", "Driver Error", ID, MSG) | |||
#define ERR_DRIVER(ID, MSG) ERRMSG("CCCL", "Driver Error", ID, MSG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flyingsilverfin @farost The reason we introduced language prefixes in codes is so that we don't run into collisions between the Rust and the host language's error codes, mainly for internal errors. INT02
coming from Java's ""Illegal state has been reached!" is distinct from Rust INT02
"Unable to send response over callback channel (receiver dropped)."
We then reasoned that there's no guarantee that Rust driver won't be emitting its own Concept errors, e.g., so the prefix got added to all Java error codes. That should have also been the case in Python. Node is the only place where collisions are irrelevant as it's not using FFI.
@@ -32,7 +32,7 @@ namespace TypeDB { | |||
|
|||
namespace DriverError { | |||
|
|||
#define ERR_DRIVER(ID, MSG) ERRMSG("CCL", "Driver Error", ID, MSG) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Git went crazy, so I duplicate the conversation here.
The latest comment from @dmitrii-ubskii:
The reason we introduced language prefixes in codes is so that we don't run into collisions between the Rust and the host language's error codes, mainly for internal errors. INT02 coming from Java's ""Illegal state has been reached!" is distinct from Rust INT02 "Unable to send response over callback channel (receiver dropped)."
We then reasoned that there's no guarantee that Rust driver won't be emitting its own Concept errors, e.g., so the prefix got added to all Java error codes. That should have also been the case in Python. Node is the only place where collisions are irrelevant as it's not using FFI.
I had a talk about it with @flyingsilverfin. We thought that we are okay with the collisions of the error codes as far as they are just ideological, not real collision problems within the code (each driver runs separately). And for each error code we have context of the driver if somebody wants to check what to do in each specific situation.
I'm okay with two ways:
- Having language-based prefix for all error codes we have in drivers
- Not having one.
I don't really like exceptions like Nodejs, because the need of differentiating FFI/non-FFI drivers only complicates things here and a user receiving such errors is not interested if a driver they are using is based on FFI or not.
Are there technical problems with having same error codes with different content for different drivers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We have checks in cloud tests that compare error codes.
- UX and error reporting suffers when users search for the error code and find something entirely different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We do not have checks that compare error codes, they would fail on factory otherwise. Only steps with "exception contains substring"
- If we care about UX, then we still have to change python and nodejs codes as well (they were outdated).
@flyingsilverfin has @dmitrii-ubskii been convincing enough to accept change to "CCDR" (cpp), "CSDR" (csharp), "JDR" (java), "PDR" (python), "NDR" (node?..) (or "TDR" as typescript?..)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean yeah i don't mind using a unique prefix for each! We can do that :)
C++ - CCDR
C# - CSDR
Java - JDR
Rust - RDR
Node - NDR
Python - PDR
If C has its own then we can use CDR, but i think it doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it does not. Okay, I proceed with this plan then, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- We do not have checks that compare error codes, they would fail on factory otherwise. Only steps with "exception contains substring"
Sorry, I meant in the typedb-cloud repo.
…#623) ## Usage and product changes We introduce packaging, distribution and documentation C# driver for TypeDB ([the original driver PR](#609)). It is built using the cross-platform [.NET 6 framework](https://dotnet.microsoft.com/en-us/download/dotnet/6.0). ### Usage: The driver is distributed as a series of [Nuget](https://www.nuget.org) packages. To use the driver, import the latest versions of the driver (TypeDB.Driver) and its Pinvoke runtime (TypeDB.Driver.Pinvoke) suitable for your platform. **CS project**: Here is an example from a `.csproj` for MacOS x86-64: ```xml <PackageReference Include="TypeDB.Driver" Version={VERSION} /> <PackageReference Include="TypeDB.Driver.Pinvoke.osx-x64" Version={VERSION} /> ``` If you aim to build a platform-independent package, reference all the needed runtimes (it will affect the size of your application by downloading a respective set of platform-specific dynamic libraries): ```xml <PackageReference Include="TypeDB.Driver.Pinvoke.osx-x64" Version={VERSION} /> <PackageReference Include="TypeDB.Driver.Pinvoke.linux-x64" Version={VERSION} /> <PackageReference Include="TypeDB.Driver.Pinvoke.win-x64" Version={VERSION} /> <PackageReference Include="TypeDB.Driver.Pinvoke.osx-arm64" Version={VERSION} /> ... ``` **Bazel**: 1. Import both the `TypeDB.Driver` and `TypeDB.Driver.Pinvoke` nuget packages as dependencies to your build rule. 2. For development, you can also use a [csharp bazel rule](https://github.com/bazelbuild/rules_dotnet/tree/master), passing targets `//csharp:driver-csharp` (the driver itself), `//csharp/Api:api` (exposed Api namespace), `//csharp/Common:common` (exposed Common namespace) as dependencies. **A simple usage example** (see `csharp/Test/Integration/Examples` for more): ``` using TypeDB.Driver.Api; using TypeDB.Driver.Common; public class TypeDBExample { public void SetupTypeDB() { string dbName = "access-management-db"; string serverAddr = "127.0.0.1:1729"; try { using (ITypeDBDriver driver = Drivers.CoreDriver(serverAddr)) { driver.Databases.Create(dbName); IDatabase database = driver.Databases.Get(dbName); using (ITypeDBSession session = driver.Session(dbName, SessionType.Schema)) { using (ITypeDBTransaction transaction = session.Transaction(TransactionType.Write)) { transaction.Query.Define("define person sub entity;").Resolve(); string longQuery = "define name sub attribute, value string; person owns name;"; transaction.Query.Define(longQuery).Resolve(); transaction.Commit(); } } database.Delete(); } } catch (TypeDBDriverException e) { Console.WriteLine($"Caught TypeDB Driver Exception: {e}"); } } } ``` ## Implementation **Documentation:** Documentation for C# is generated in a similar way as the C/C++ ones: `doxygen` -> `adoc`. This way, the C# parser has some extra logic of parsing the intermediate doxygen files but follows a similar (copypasted) structure. Additionally, some small bugs for both old parsers have been fixed. **Distribution:** We decided to distribute the C# driver as a series of packages to allow the end user to choose between multi-platform and compactness. This way, we have: `TypeDB.Driver.{X-X-X-version}.nupkg` - the main package with the driver, which fails in runtime if no runtime is additionally provided. `TypeDB.Driver.Pinvoke.{X-X-X-version}.{platform}.nupkg` - platform-specific runtimes (`osx-x64`, `osx-arm64`, `linux-x64`, `linux-arm64`, `win-x64`). The distribution is implemented via several bazel rules: `nuget_pack` (with a wrapper for native packages in this PR) and `nuget_push` and their respective calls for both runtime and main packages. **Tests:** Integration tests use NUnit framework as a built-in framework for C# bazel test rules. This PR presents both `test-core` and `test-cloud` as small usage examples within the integration tests. These tests are also triggered by Factory. There is also a new Deployment test, which creates a `.csproj` project and tries to fetch all the needed (and all the available runtime) packages to use them for another small C# program. This test is triggered by CircleCI.
Usage and product changes
We introduce the C# driver for TypeDB. It is built using the cross-platform .NET 6 framework.
Usage: Deployment and usage examples will be provided in a separate pull request. Current state of the code lets you compiling the driver + writing and running behaviour and integration tests for it.
The driver is expected to be a Nuget package, which could be added as a dependency to a project and referenced via "using" statements inside the users' code for all platforms.
Architecture: The C# driver is a thin wrapper around the TypeDB Rust driver, introducing classes for a more intuitive interface. Mostly each C# object holds a reference to the corresponding native Rust object, using an FFI (SWIG for C#) for the native object wrappers generation and resource management.
Any error encountered will throw a
TypeDBDriverException
. Note that methods which return anIEnumerable
or a Promise and encounter a server-side error will only throw when the return objects are evaluated (e.g. iterate over or call aLinq
method for anIEnumerable
and callResolve()
for aPromise
).A simple usage example (more to come after the integration tests are finished in another PR):
Implementation
Driver:
Introduces the C# driver, wrapping the C-native classes produced by Rust driver using SWIG. The driver aims to provide a familiar interface for C# developers, hiding data transformations and native calls under the hood.
The code's architecture and build flow follow the one from Java with small C#-specific alterations.
Tests:
Integration tests use NUnit framework as a built-in framework for C# bazel test rules (more to come in another PR, right now these tests are messy).
Behaviour tests use Xunit.Gherkin.Quick framework and are implemented using partial classes for flexible construction of tests based on different sets of step declaration files.
Build:
To fetch nuget packages, we use Paket and reference this dependencies in a regular Bazel way (example from the official Bazel repo for C# rules).
The corresponding dependencies PR for C# rules is here.