Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add support for custom headers for REST via config. #5104

Merged
merged 9 commits into from
Jun 19, 2024
Merged

Conversation

teo-tsirpanis
Copy link
Member

SC-49141

Similar to S3 custom header support (#4400) this PR adds support for custom headers in REST request. This allows users to set header via a config parameter instead of the current tildb_context_set_tag. The context set tag does not work in all cases because its not always possible to use the same context, i.e SOMA and VCF python APIs can't pass a TileDB-Py ctx to their respective C++ code.

Big thanks to @Shelnutt2 for implementing this. I added a test and documentation.


TYPE: CONFIG
DESC: Add rest.custom_headers.* config option to set custom headers on REST requests.

Shelnutt2 and others added 3 commits June 18, 2024 16:59
Similar to S3 custom header support this PR adds support for customer
header in REST request. This allows users to set header via a config
parameter instead of the current `tildb_context_set_tag`. The context
set tag does not work in all cases because its not always possible to
use the same context, i.e SOMA and VCF python APIs can't pass a
TileDB-Py ctx to their respective C++ code.
Copy link
Contributor

@eric-hughes-tiledb eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

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

The change suggestions make this PR more compatible with #4994.

  • Returning a map requires copying it. It's inefficient and verbose when an in-place initialization in (effectively) the constructor would work just as well.
  • The friend declaration is unnecessary when exposing extra_headers as a constant accessor work just as well.

tiledb/sm/rest/rest_client.h Show resolved Hide resolved
tiledb/sm/rest/rest_client.h Outdated Show resolved Hide resolved
test/src/unit-curl.cc Outdated Show resolved Hide resolved
test/src/unit-curl.cc Outdated Show resolved Hide resolved
tiledb/sm/rest/rest_client.h Outdated Show resolved Hide resolved
tiledb/sm/rest/rest_client.cc Outdated Show resolved Hide resolved
tiledb/sm/rest/rest_client.cc Outdated Show resolved Hide resolved
tiledb/sm/rest/rest_client.cc Outdated Show resolved Hide resolved
tiledb/sm/rest/rest_client.cc Outdated Show resolved Hide resolved
KiterLuc and others added 2 commits June 19, 2024 15:13
Co-authored-by: eric-hughes-tiledb <82400964+eric-hughes-tiledb@users.noreply.github.com>
@eric-hughes-tiledb
Copy link
Contributor

Is this PR superseded by #5105, or are we going to merge both?

@KiterLuc KiterLuc dismissed eric-hughes-tiledb’s stale review June 19, 2024 15:14

All changes addressed.

@KiterLuc KiterLuc merged commit 6539f14 into dev Jun 19, 2024
61 checks passed
@KiterLuc KiterLuc deleted the rest-headers branch June 19, 2024 15:14
KiterLuc added a commit that referenced this pull request Jun 19, 2024
[SC-49141](https://app.shortcut.com/tiledb-inc/story/49141/add-support-for-custom-rest-headers-via-config)

Similar to S3 custom header support (#4400) this PR adds support for
custom headers in REST request. This allows users to set header via a
config parameter instead of the current `tildb_context_set_tag`. The
context set tag does not work in all cases because its not always
possible to use the same context, i.e SOMA and VCF python APIs can't
pass a TileDB-Py ctx to their respective C++ code.

Big thanks to @Shelnutt2 for implementing this. I added a test and
documentation.

---
TYPE: CONFIG
DESC: Add `rest.custom_headers.*` config option to set custom headers on
REST requests.

---------

Co-authored-by: Seth Shelnutt <seth@tiledb.io>
Co-authored-by: KiterLuc <67824247+KiterLuc@users.noreply.github.com>
Co-authored-by: eric-hughes-tiledb <82400964+eric-hughes-tiledb@users.noreply.github.com>
Co-authored-by: Luc Rancourt <lucrancourt@gmail.com>
@KiterLuc
Copy link
Contributor

Is this PR superseded by #5105, or are we going to merge both?

Both will be merged everywhere today.

KiterLuc added a commit that referenced this pull request Jun 19, 2024
[SC-49141](https://app.shortcut.com/tiledb-inc/story/49141/add-support-for-custom-rest-headers-via-config)

Similar to S3 custom header support (#4400) this PR adds support for
custom headers in REST request. This allows users to set header via a
config parameter instead of the current `tildb_context_set_tag`. The
context set tag does not work in all cases because its not always
possible to use the same context, i.e SOMA and VCF python APIs can't
pass a TileDB-Py ctx to their respective C++ code.

Big thanks to @Shelnutt2 for implementing this. I added a test and
documentation.

---
TYPE: CONFIG
DESC: Add `rest.custom_headers.*` config option to set custom headers on
REST requests.

---------

Co-authored-by: Seth Shelnutt <seth@tiledb.io>
Co-authored-by: KiterLuc <67824247+KiterLuc@users.noreply.github.com>
Co-authored-by: eric-hughes-tiledb <82400964+eric-hughes-tiledb@users.noreply.github.com>
Co-authored-by: Luc Rancourt <lucrancourt@gmail.com>
KiterLuc added a commit that referenced this pull request Jun 19, 2024
…onfig. (#5104) (#5111)

Backport 6539f14 from #5104.

---
TYPE: CONFIG
DESC: Add `rest.custom_headers.*` config option to set custom headers on
REST requests.

Co-authored-by: Theodore Tsirpanis <theodore.tsirpanis@tiledb.com>
Co-authored-by: Seth Shelnutt <seth@tiledb.io>
Co-authored-by: eric-hughes-tiledb <82400964+eric-hughes-tiledb@users.noreply.github.com>
KiterLuc added a commit that referenced this pull request Jun 19, 2024
…onfig. (#5104) (#5122)

Backport 6539f14 from #5104.

---
TYPE: CONFIG
DESC: Add `rest.custom_headers.*` config option to set custom headers on
REST requests.

---------

Co-authored-by: Theodore Tsirpanis <theodore.tsirpanis@tiledb.com>
Co-authored-by: Seth Shelnutt <seth@tiledb.io>
Co-authored-by: eric-hughes-tiledb <82400964+eric-hughes-tiledb@users.noreply.github.com>
@teo-tsirpanis
Copy link
Member Author

Returning a map requires copying it.

@eric-hughes-tiledb I don't understand the "requires" part. Consider the following snippet:

struct C {
    C() = default;
    C(const C&) = delete;
    C(C&&) = default;
};

C foo() {
    C c;
    return c;
}

It compiles in all three major compilers, whereas if I delete the move constructor it does not.

Regardless, for my own learning, is there a way to guarantee that a return value is moved? I have tried return std::move(x) but got a warning-turned-error saying I should just return x.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants