-
Notifications
You must be signed in to change notification settings - Fork 2
Initial draft of find / find one #8
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
Conversation
…ology, switch integrations tests to xunit, begin adding db administration methods
Db admin - initial commit
added ListDatabases, ListDatabasesNames and tests
added DoesDatabaseExist by string and by guid
…check, and list collections, rounding out create collection methods
Database admin
# Conflicts: # src/DataStax.AstraDB.DataAPI/Core/DatabaseOptions.cs # src/DataStax.AstraDB.DataApi/Admin/AstraDatabasesAdmin.cs # src/DataStax.AstraDB.DataApi/Admin/CloudProviderType.cs # src/DataStax.AstraDB.DataApi/Collections/Collection.cs # src/DataStax.AstraDB.DataApi/Core/CommandOptions.cs # src/DataStax.AstraDB.DataApi/Core/CommandUrlBuilder.cs # src/DataStax.AstraDB.DataApi/Core/Commands/Command.cs # src/DataStax.AstraDB.DataApi/Core/Database.cs # src/DataStax.AstraDB.DataApi/Core/DatabaseOptions.cs # src/DataStax.AstraDB.DataApi/Core/Extensions.cs # src/DataStax.AstraDB.DataApi/DataAPIClient.cs # src/DataStax.AstraDB.DataApi/Utils/Extensions.cs # src/DataStax.AstraDB.DataApi/Utils/Guard.cs # test/DataStax.AstraDB.DataApi.IntegrationTests/AdminFixture.cs # test/DataStax.AstraDB.DataApi.IntegrationTests/ClientFixture.cs # test/DataStax.AstraDB.DataApi.IntegrationTests/Tests/AdminTests.cs # test/DataStax.AstraDB.DataApi.IntegrationTests/Tests/CollectionTests.cs
# Conflicts: # src/DataStax.AstraDB.DataApi/Admin/DatabaseAdminNonAstra.cs # src/DataStax.AstraDB.DataApi/Collections/Collection.cs # src/DataStax.AstraDB.DataApi/Collections/CollectionInsertManyResult.cs # src/DataStax.AstraDB.DataApi/Core/Commands/Command.cs # src/DataStax.AstraDB.DataApi/Core/Extensions.cs # src/DataStax.AstraDB.DataApi/Core/Results/ListCollectionsResult.cs # src/DataStax.AstraDB.DataApi/Utils/Guard.cs
Initial draft of find / find many
|
|
||
| public class InsertManyOptions | ||
| { | ||
| public const int MaxChunkSize = 50; |
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.
Try to avoid hardcoding Data API limits into the client (since they may change in the future, so it's best to just let the Data API deal with any limit issues)
Also, the Data API's batch size limit is currently 100, not 50 🙃. It's just recommended for the clients to keep it at 50 for better average performance
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.
Ok, I was following the Java client there. I see the TS client defaults to 50 but doesn't enforce a max.
| return new FluentFind<T, TId, TResult>(this, filter); | ||
| } | ||
|
|
||
| public Cursor<T> FindMany() |
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 is no findMany; just find (adhering with the Data API)
Also, creating a cursor (e.g. Collection.Find() should always be pure; no I/O should be performed until the cursor is actually used.
- Meaning, the first page should only be fetched when a method like
.Next(),.HasNext(),.ToArray(), etc. is called
Also it should be named CollectionFindCursor here, which should extend a base FindCursor, which in turn extends AbstractCursor (mermaid)
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've reworked the method naming, adjusted the calls to wait until the data is accessed. Will consider potential refactoring base cursor once we implement tables.
| return response.Data.Document; | ||
| } | ||
|
|
||
| public FluentFind<T, TId, T> Find() |
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.
FluentFind's properties/methods should probably be parts of the FindCursor object itself
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 is somewhat modified in the next merge.
|
|
||
| T DeserializeFromMap<T>(Dictionary<string, object> map); | ||
| } | ||
| public List<T> InsertedIds { get; internal set; } = new List<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.
Should have an InsertedCount property as well
i.e.
public int InsertedCount => InsertedIds.Count;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.
Added.
| } | ||
|
|
||
| internal class ApiResponseDictionary : Dictionary<string, object> | ||
| internal class ApiResponseWithData<T, TStatus> |
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 may very well know this already, but just as a heads up that there may be a warnings field present as well, alongside data, status, and errors
warnings will have the exact same response shape as errors.
- Only point of note is that the
scopeof a warning is always'WARNING'
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 for the note, this has been added.
|
|
||
| namespace DataStax.AstraDB.DataApi.Core.Query; | ||
|
|
||
| public class FilterBuilder<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.
There should probably be an escape hatch function for filter operators which may be added in the future (unless the escape hatch is creating a new Filter object manually, in which case, disregard)
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.
Hmm, interesting consideration. Will think about.
No description provided.