CDRIVER-3422 add entity map and BSON parser #712
Conversation
kevinAlbs
changed the title
kevinAlbs
force-pushed
the
unified-part3.CDRIVER-3422
branch
from
266e7ab to
274a541
Compare
| LL_FOREACH_SAFE (parser->entries, entry, tmp) | ||
| { | ||
| /* Destroy alternates. */ | ||
| LL_FOREACH_SAFE (entry->alternates, alternate, tmp2) |
There was a problem hiding this comment.
Can alternates ever have alternates? My gut says that if we ended up in that situation, something's gone wrong. I think we should check for it, though, or handle it another way. The bson_parser_entry_destroy method could BSON_ASSERT that entry->alternates is NULL, or could free its own alternatives instead of this method here freeing the alternatives (which might be a more natural place to free an entry's alternatives anyway)
There was a problem hiding this comment.
Correct, alternates cannot have alternates. I added a check in bson_parser_parse to assert when iterating over the alternates that they have alternates set to NULL.
| e->key = bson_strdup (key); | ||
|
|
||
| if (alternate) { | ||
| /* There should already be an entry. Add this to the list of alternates. |
There was a problem hiding this comment.
I think it would feel better as a test implementor to label all the alternatives as you go, rather than needing to make the first entry different. Could we change this logic to search for an existing alternative first, then fall back to treating this as a new entry?
This method could be restructured to search for the existing key first. If 'key is found and this isn't an alternate, error. If it is an alternate, then add as an alternate. If key is not found, add in the new entry.
There was a problem hiding this comment.
Oh good idea. I went with that approach. If adding an alternate and no existing entry exists, a new entry is created.
| bson_parser_allow_extra (bson_parser_t *bp, bool val); | ||
|
|
||
| void | ||
| bson_parser_destroy (bson_parser_t *bp); |
There was a problem hiding this comment.
Do you think we'd ever have a use for a "clear" method that reset the parser to a state where it could be used on new bson?
There was a problem hiding this comment.
I could see that potentially being useful. I think the "clear" would only need to destroy all output parameters and NULL them.
| parser = bson_parser_new (); | ||
| bson_parser_utf8 (parser, "id", &entity->id); | ||
| bson_parser_utf8 (parser, "client", &client_id); | ||
| bson_parser_doc (parser, "sessionOptions", &session_opts_bson); |
There was a problem hiding this comment.
It looks like sessionOptions should be an optional document, not a required one
| if (!entity) { | ||
| return; | ||
| } | ||
| if (entity->type) { |
There was a problem hiding this comment.
when would we expect to have an entity that didn't have a type?
There was a problem hiding this comment.
Good question. Entities should always have a type. I changed this to an assertion.
src/libmongoc/tests/unified/util.c
Outdated
| char* w_string; | ||
| int64_t *wtimeoutms; | ||
|
|
||
| parser = bson_parser_new (); |
There was a problem hiding this comment.
I feel like this bson_parser is already proving to be so handy!
There was a problem hiding this comment.
Yeah, I hope it is useful!
src/libmongoc/tests/unified/util.h
Outdated
|
|
||
| #include "mongoc/mongoc.h" | ||
|
|
||
| mongoc_write_concern_t * bson_to_write_concern (bson_t *bson, bson_error_t* error); |
There was a problem hiding this comment.
I feel like this is a slightly different style from our normal header declarations? Clang format?
There was a problem hiding this comment.
Ah I did forget to run clang-format. Done.
| #include "entity-map.h" | ||
|
|
||
| #include "bson-parser.h" | ||
| #include "TestSuite.h" |
Summary of changes:
bson_parser_tmake test parsing code consistent and safer. By default, extra fields are considered an error.Notes:
client,database, andcollectionentity types. Support forbucketandsession.defaultTransactionOptionswill be added when needed.