Options rework#187
Conversation
|
Here is my opinion:
Yes
I'd say no, but am less sure here. Do you have specific cases in mind where a type parameter might become useful in the future?
Good point. Having users pass an explicitly
I've got nothing against it.
Very good point. I am reviewing the current location of existing |
|
@toptobes Here is my assessment of a possible more-or-less consistent structure for the various Options files. We can start from here and refine as needed. Sections here are the desired location, the file path+name in the lists is the current one (as of Notes:
DELETE (these seem unused?)
IN
IN
IN Are we ok to keep all things "Query" here? (rather than stick to the core/coll/tables like the rest?) I'm OK with that._
IN
IN
IN
|
|
|
as for the location of the options objects I'll come back to this at the end |
| /// <summary> | ||
| /// Returns a single document from the collection based on the provided <see cref="CollectionFindOneOptions{T}"/>. | ||
| /// This will return the first document found, most often used in conjunction with <see cref="CollectionFindOneOptions{T}.Sort"/>. | ||
| /// See <see cref="CollectionFindOneOptions{T}"/> for more details on sorting, projecting and the other options for finding a document. |
There was a problem hiding this comment.
While the CollectionFindOneOptions{T}.Sort reference in line above this one is an actual warning (apparently inherited attributes/methods aren't tracked), a line line this one, while not giving warnings, would arguably need to be changed to refer to the base BaseFindOptions perhaps?
(Note: I know you aren't looking at xmldocs warnings yet, this is more a remark to plan the closing work for this PR. Which incidentally I can volunteer to do myself if you have better things to do).
There was a problem hiding this comment.
CollectionFindOneOptions will end up inheriting docs from BaseFindOneOptions
| findOptions = findOptions.WithFilterParam(filter); | ||
| var command = CreateCommand("findOne").WithPayload(findOptions).AddCommandOptions(commandOptions); | ||
| findOptions ??= new CollectionFindOneOptions<T>(); | ||
| var commandOptions = findOptions; |
There was a problem hiding this comment.
Is there a reason why this method directly uses the findOptions if provided, while e.g. InsertManyAsync merges the options tree (above, line 194), putting the method arg on top of the pile?
I mean, options such as timeouts, etc can potentially be specified at either level and apply to both method. (This is a broader question, probably, about whether the "merge the options tree" pattern should be ubiquitous).
(Which in itself raises a more general question. What is the expected behaviour if invoking e.g. FindOneAsync one passes a different Environment or Destination in the CollectionFindOneOptions?
Note that some kind of "misplaced layer for options" occurs in other clients already and is not a problem in itself. E.g. in astrapy you can pass a whole spawn_api_options when creating a Collection off a Database, including table-specific serdes settings - which of course won't be ever looked at. So "the new env/dest is ignored" is probably a valid answer. This comment originated by looking at this line but has wide scope).
There was a problem hiding this comment.
outdated – options were in a bit of a transitional stage as I was sorting out some bugs
What is the expected behaviour if invoking e.g. FindOneAsync one passes a different Environment or Destination in the CollectionFindOneOptions?
this is very much a more general question and I'm not sure it's in the scope of this PR. Wdyt should happen though?
There was a problem hiding this comment.
this is very much a more general question and I'm not sure it's in the scope of this PR.
Agree. I'd also add that this could realistically be pushed postGA.
Wdyt should happen though?
Ignoring the new settings sounds inelegant. Probably should throw.
Maybe a good, centralized solution could be to restrict the options Merge operation to "mergeable fields" (timeouts, etc), and refuse to merge when other fields are passed.
(and in case a "major merge" is used, maybe when creating a client - I don't recall, there may be an additional MergeComprehensively method, or something like that).
| { | ||
| throw new ArgumentException("Cannot run ordered insert_many concurrently."); | ||
| } | ||
|
|
||
| foreach (var doc in documents) | ||
| { | ||
| InsertValidator.Validate(doc); |
There was a problem hiding this comment.
This is another off-topic (sorry). Do we want this validation to happen?
It probably consumes very little cpu cycles in the whole "insertion" latency budget, however it might be not a client's job, at least to an extent.
For example, null is a valid _id for the API, and so is "" (I think).
If you agree, this item would become a postGA ticket.
There was a problem hiding this comment.
heh I was this close (🤏) from removing it myself, I'm also not sure about keeping that validation
There was a problem hiding this comment.
It looks like there's nothing that wouldn't be caught by the server and returned as an error in the response (as the general clients' philosophy would prefer).
Except those not-real-errors that we noted above.
If it's convenient to do it in this branch, feel free to remove 🤷
| /// <returns></returns> | ||
| /// <param name="filter">The filter to match documents.</param> | ||
| /// <param name="options">Options for the delete operation.</param> | ||
| /// <returns>The result of the delete operation.</returns> | ||
| /// <remarks> | ||
| /// Deleting all documents in a collection is not recommended for large collections. However, if needed | ||
| /// you can pass null as the filter to delete all documents in the collection. |
There was a problem hiding this comment.
while you're at it, you might change this line to suggest usage of CollectionFilter.Empty()
(esp. since we plan to reject actual nulls for safety).
| /// and that an exception will be thrown by this method if this limit is encountered. | ||
| /// </remarks> | ||
| /// <throws cref="DocumentCountExceedsMaxException">Thrown if the number of documents to count exceeds the limit</throws> | ||
| public Task<int> CountDocumentsAsync(int maxDocumentsToCount) |
There was a problem hiding this comment.
@skedwards88 I believe this being removed (the overload with just the upper bound) means the docs example under astra-db-serverless/api-reference/document-methods/count-all.html#count-all-documents will need to get a CollectionFilters.Empty() as first parameter.
(this is intentional, to encourage users to be conscious about counting all on large collections)
| } | ||
|
|
||
| [Fact] | ||
| [SkipWhenNotAstra] // TODO why is this not throwing on HCD in CI? It throws just fine when running on HCD locally |
There was a problem hiding this comment.
Probably related (albeit in reverse, local<-->runner) to what I experience locally, namely that on my laptop when I run Data API + HCD there's no actual user/pwd check. Anything I pass would just work. (I always blamed podman for failing in mounting some cassandra.yaml, but never managed to get to the bottom of it).
| Guard.NotNull(row, nameof(row)); | ||
|
|
||
| options = options?.ShallowCopy() ?? new(); | ||
| SetRowSerializationOptions<T>(options, true); |
There was a problem hiding this comment.
Another tangential thought is that SetRowSerializationOptions does act immutably, i.e.: "if it weren't for the fact that it reads the type of Row, it could be a static method".
I wonder if it makes sense to make it into one, with Row an explicit parameter. That would give compile-time guarantees against future bugs that end up mutating something in the instance (not a real concern, but ... )
2 participants
still fleshing it out but some questions include:
= nullfor options so we don't need as many overloads