Add group member with type #5336
Conversation
| absolute_group_member_uri = | ||
| group_uri_.join_path(group_member_uri.to_string()); | ||
| } | ||
| obj_type = object_type(resources, absolute_group_member_uri); |
There was a problem hiding this comment.
Does this not re-introduce the problem?
There was a problem hiding this comment.
Only if the object type is not specified by the user. We can move the item type check later, at the time of persisting the group details (and parallelize it and do it on the REST server for remote groups); I think I've brought this up but don't remember what we decided about it.
There was a problem hiding this comment.
As Teo said that call is in the else branch which is only hit when a type is not provided.
As for moving the check later this is out of the scope of this PR, @teo-tsirpanis feel free to open a ticket about your suggestions.
| const char* uri, | ||
| const uint8_t relative, | ||
| const char* name, | ||
| tiledb_object_t type) TILEDB_NOEXCEPT; |
There was a problem hiding this comment.
It would be nice if there was a special TILEDB_AUTODETECT value that could be passed in this parameter. This would allow us to call this function add_member_v2, and entirely supersede the old one.
ypatia
force-pushed
the
yt/sc-47918/add_group_member_by_type
branch
from
7a6d039
to
02e3d3f
Compare
ypatia
force-pushed
the
yt/sc-47918/add_group_member_by_type
branch
from
c706cce
to
1e7e70e
Compare
ypatia
force-pushed
the
yt/sc-47918/add_group_member_by_type
branch
from
4d9add2
to
5b3f08f
Compare
ypatia
force-pushed
the
yt/sc-47918/add_group_member_by_type
branch
from
5b3f08f
to
53e5d39
Compare
ypatia
force-pushed
the
yt/sc-47918/add_group_member_by_type
branch
from
a89b6dc
to
3186b6e
Compare
|
I felt a good citizen in this PR and decided to port relevant UTs to REST CI. After spending too much time debugging issues unrelated to this PR, I decided to go with disabling a few of those test to be able to let this change go in finally since it's an important one. |
test/src/unit-cppapi-group.cc
Outdated
|
|
||
| // Set expected | ||
| auto uri_format = [&](std::string uri) { | ||
| return vfs_test_setup_.is_rest() ? vfs_test_setup_.with_prefix(uri) : uri; |
There was a problem hiding this comment.
Just an idea from looking at CI.. maybe this is flipped around and we want to use with_prefix for the false case?
There was a problem hiding this comment.
I removed all that with_prefix logic that was confusing and went back to using URIs.
ypatia
force-pushed
the
yt/sc-47918/add_group_member_by_type
branch
from
d1df508
to
22b550f
Compare
Add a new
tiledb_group_add_member_with_typeAPI to avoid checking for the type of the asset before adding a member to a group for optimization purposes.[sc-47918]
TYPE: C_API
DESC: Add API to get group member with type
TYPE: CPP_API
DESC: Update add_member API to accept an optional type