Skip to content
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

Merged
merged 148 commits into from
Mar 22, 2024
Merged

Conversation

farost
Copy link
Member

@farost farost commented Mar 15, 2024

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 an IEnumerable or a Promise and encounter a server-side error will only throw when the return objects are evaluated (e.g. iterate over or call a Linq method for an IEnumerable and call Resolve() for a Promise).

A simple usage example (more to come after the integration tests are finished in another PR):

// Inside a try-catch block
using (ITypeDBDriver driver = TypeDB.CoreDriver(TypeDB.DEFAULT_ADDRESS))
{
    string dbName = "mydb";
    driver.Databases.Create(dbName);
    IDatabase mydb = driver.Databases.Get(dbName);
    System.Console.WriteLine(mydb.Name);

    using (ITypeDBSession schemaSession = driver.Session(dbName, SessionType.SCHEMA))
    {
        using (ITypeDBTransaction writeTransaction = schemaSession.Transaction(TransactionType.WRITE))
        {
            string defineQuery = "...some define query...";
            writeTransaction.Query.Define(defineQuery).Resolve();
            writeTransaction.Commit();
        }
    }

    mydb.Delete();
}

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.

@typedb-bot
Copy link
Member

typedb-bot commented Mar 15, 2024

PR Review Checklist

Do 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

  • This change is trivial and does not require a code or architecture review.

Code

  • Packages, classes, and methods have a single domain of responsibility.
  • Packages, classes, and methods are grouped into cohesive and consistent domain model.
  • The code is canonical and the minimum required to achieve the goal.
  • Modules, libraries, and APIs are easy to use, robust (foolproof and not errorprone), and tested.
  • Logic and naming has clear narrative that communicates the accurate intent and responsibility of each module (e.g. method, class, etc.).
  • The code is algorithmically efficient and scalable for the whole application.

Architecture

  • Any required refactoring is completed, and the architecture does not introduce technical debt incidentally.
  • Any required build and release automations are updated and/or implemented.
  • Any new components follows a consistent style with respect to the pre-existing codebase.
  • The architecture intuitively reflects the application domain, and is easy to understand.
  • The architecture has a well-defined hierarchy of encapsulated components.
  • The architecture is extensible and scalable.

@farost
Copy link
Member Author

farost commented Mar 15, 2024

A quick comment about Naming

A typical class's signature is:

class ClassName
{
    private int _privateField;
    public int PublicField;

    void MethodName(int argumentName)
    {
        ...
    }
}

C# identifier names coding conventions
C# common code 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:
I try to follow what Microsoft says, considering Haikal's recent comment: "Please don't ever spell it Typedb":
C# Names of Namespaces

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:
bazel test //java/test/behaviour/connection/transaction:test-core;
We need to uppercase everything inside the csharp's path:
bazel test //csharp/Test/Behaviour/Connection/Transaction:test-core;

Copy link
Member Author

@farost farost left a 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.

csharp/Api/Answer/IConceptMap.cs Show resolved Hide resolved
%}


%typemap(csbase) Error "System.Exception";
Copy link
Member Author

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()
Copy link
Member Author

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 %{
Copy link
Member Author

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)
Copy link
Member Author

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.

java/test/behaviour/query/QuerySteps.java 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());
Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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/BUILD 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
Copy link
Member Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

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.

Copy link
Member

@dmitrii-ubskii dmitrii-ubskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review

c/swig/typedb_driver_csharp.swg Outdated Show resolved Hide resolved
@@ -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).
Copy link
Member

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",
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member Author

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.

Copy link
Member

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#?

Copy link
Member Author

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.Apiand... Uhm... TypeDB.Driver?


csharp_library(
name = "api",
srcs = glob(["*.cs", "*/*.cs", "*/*/*.cs"]),
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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);
Copy link
Member

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?


public class ValueType : NativeObjectWrapper<Pinvoke.ValueType>
{
public static readonly ValueType OBJECT = new ValueType(typeof(object), false, false, Pinvoke.ValueType.Object);
Copy link
Member

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 ValueTypes here, and turn ValueClass etc. into get properties?

csharp/Api/Concept/Value/IValue.cs Outdated Show resolved Hide resolved
* driver.Databases.All;
* </pre>
*/
IList<IDatabase> All { get; }
Copy link
Member

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.

* conceptMap.Variables;
* </pre>
*/
IEnumerable<string> Variables { get; }
Copy link
Member

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

csharp/Api/TypeDBCredential.cs Show resolved Hide resolved
Copy link
Member Author

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!

*/
Promise<bool> IsDeleted(ITypeDBTransaction transaction);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛔️❗️

*/
Promise<bool> IsDeleted(ITypeDBTransaction transaction);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check IDEA settings.


public class ValueType : NativeObjectWrapper<Pinvoke.ValueType>
{
public static readonly ValueType OBJECT = new ValueType(typeof(object), false, false, Pinvoke.ValueType.Object);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are Pinvoke.ValueTypes not integers?

Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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());
Copy link
Member

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.

Copy link
Member

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"?

Copy link
Member Author

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;
Copy link
Member Author

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).

Copy link
Member

@flyingsilverfin flyingsilverfin left a 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.

WORKSPACE Show resolved Hide resolved
public $csclassname Released()
{
var cPtr = swigCPtr.Handle;
if (swigCMemOwn)
Copy link
Member

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?

Copy link
Member Author

@farost farost Mar 21, 2024

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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

Copy link
Member Author

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...

c/swig/typedb_driver_csharp.swg Show resolved Hide resolved
%promise(String, string, string)
%promise(Bool, bool, bool)

/* void promises require special handling */
Copy link
Member

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)
Copy link
Member

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!

Copy link
Member Author

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.

csharp/BUILD Show resolved Hide resolved
# 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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

java/api/concept/value/Value.java Show resolved Hide resolved
public void SendArraysToNativeSide()
{
string dbName = "mydb";
ITypeDBDriver driver = Utils.OpenConnection();
Copy link
Member

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.

Copy link
Member Author

@farost farost Mar 21, 2024

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!

csharp/Test/Integration/Memory/MemoryTest.cs Outdated Show resolved Hide resolved
@farost farost mentioned this pull request Mar 21, 2024
@farost farost force-pushed the csharp-kickoff branch 7 times, most recently from 95d1436 to 914ff51 Compare March 21, 2024 17:55
public $csclassname Released()
{
var cPtr = swigCPtr.Handle;
if (swigCMemOwn)
Copy link
Member

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.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From #610

Suggested change
sudo add-apt-repository ppa:deadsnakes/ppa
sudo add-apt-repository -y ppa:deadsnakes/ppa

Copy link
Member Author

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

.factory/automation.yml Outdated Show resolved Hide resolved
arrayPtr[i] = global::System.Runtime.InteropServices.Marshal.StringToCoTaskMemAnsi($csinput[i]);
}

arrayPtr[i] = global::System.IntPtr.Zero;
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -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)
Copy link
Member

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)
Copy link
Member Author

@farost farost Mar 22, 2024

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?

Copy link
Member

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.

Copy link
Member Author

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?..)?

Copy link
Member

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?

Copy link
Member Author

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!

Copy link
Member

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.

@farost farost merged commit a3041f1 into typedb:development Mar 22, 2024
23 checks passed
@farost farost deleted the csharp-kickoff branch March 22, 2024 15:20
@farost farost restored the csharp-kickoff branch March 25, 2024 08:58
farost added a commit that referenced this pull request Apr 4, 2024
…#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants