Skip to content
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

Small fix: Improve error message when not providing provider key through x-embedding-api-key #1251

Merged
merged 4 commits into from
Jul 10, 2024

Conversation

Hazel-Datastax
Copy link
Contributor

@Hazel-Datastax Hazel-Datastax commented Jul 9, 2024

What this PR does:
Small fix: Improve error message when not providing provider key through x-embedding-api-key

Which issue(s) this PR fixes:
Fixes #1250

Checklist

  • Changes manually tested
  • Automated Tests added/updated
  • Documentation added/updated
  • CLA Signed: DataStax CLA

@Hazel-Datastax Hazel-Datastax requested a review from a team as a code owner July 9, 2024 19:58
@@ -115,6 +115,7 @@ public Uni<Response> vectorize(
List<String> texts,
Optional<String> apiKeyOverride,
EmbeddingRequestType embeddingRequestType) {
checkEmbeddingApiKeyHeader(providerId, apiKeyOverride);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is override really always needed? (maybe it's just odd name for parameter and means "apiKey"?)

@tatu-at-datastax
Copy link
Contributor

As long as Optional apiKeyOverride is really always needed, makes sense.
But confused about "override" part of the name -- if that's really just the key that is needed, fine, otherwise override would be optional I think.

Also come to think of that... if it's required, why would we use Optional<> at all?
Shouldn't be non-null value?

@Hazel-Datastax
Copy link
Contributor Author

The name "override" is a legacy term. I've updated it accordingly.

I think changing Optional<String> to String might be too aggressive. All apiKey values come from getEmbeddingApiKey() in DataApiRequestInfo (

public Optional<String> getEmbeddingApiKey() {
). Instead of adding a checker in each embedding provider class, we could modify getEmbeddingApiKey() as follows:

  public String getEmbeddingApiKey() {
    if (embeddingApiKey.isEmpty()) {
      throw EMBEDDING_PROVIDER_API_KEY_MISSING.toApiException(
              "header value `%s` is missing in the request.",
              EMBEDDING_AUTHENTICATION_TOKEN_HEADER_NAME);
    }
    return embeddingApiKey.get();
  }

However, I think this change would limit the flexibility of the getEmbeddingApiKey() function, even though it is only used to pass the header value to each embedding provider class.

@tatu-at-datastax WDYT? Adding checker in each class or changing getEmbeddingApiKey()

@Hazel-Datastax Hazel-Datastax merged commit 197bfd2 into main Jul 10, 2024
4 checks passed
@Hazel-Datastax Hazel-Datastax deleted the hazel/add_checker branch July 10, 2024 19:12
@tatu-at-datastax
Copy link
Contributor

The name "override" is a legacy term. I've updated it accordingly.

I think changing Optional<String> to String might be too aggressive. All apiKey values come from getEmbeddingApiKey() in DataApiRequestInfo (

public Optional<String> getEmbeddingApiKey() {

). Instead of adding a checker in each embedding provider class, we could modify getEmbeddingApiKey() as follows:

  public String getEmbeddingApiKey() {
    if (embeddingApiKey.isEmpty()) {
      throw EMBEDDING_PROVIDER_API_KEY_MISSING.toApiException(
              "header value `%s` is missing in the request.",
              EMBEDDING_AUTHENTICATION_TOKEN_HEADER_NAME);
    }
    return embeddingApiKey.get();
  }

However, I think this change would limit the flexibility of the getEmbeddingApiKey() function, even though it is only used to pass the header value to each embedding provider class.

@tatu-at-datastax WDYT? Adding checker in each class or changing getEmbeddingApiKey()

I like centralized validation: but you can split this in 2 -- "plain" getter that simply returns value (letting caller validate), then "getAndValidateEmbeddingApiKey()" that validates and gives error? Second method calls first to get value, then checks validity.

Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

amorton pushed a commit that referenced this pull request Jul 12, 2024
amorton pushed a commit that referenced this pull request Jul 12, 2024
amorton added a commit that referenced this pull request Jul 16, 2024
This commit gets the insert and find working in limited cases.

Find does not support any clauses, just returns everything. Insert
expects the document to have a field called `key`

The feature flag to enable tables has been renamed to fit convention, it
is now `-Dstargate.tables.enabled=true`

Full command to start quarkus in dev against HCD container would be:

./mvnw quarkus:dev -Dstargate.data-store.ignore-bridge=true \
-Dstargate.jsonapi.operations.vectorize-enabled=true \
-Dstargate.jsonapi.operations.database-config.local-datacenter=dc1 \
-Dquarkus.log.console.darken=2 -Dstargate.tables.enabled=true -Poffline

This is the 3rd chunk of changes, it is built against the work in
ajm/tables-chunk-2 branch.

it pulls commits from the ajm/tables branch listed below:

commit e35c78b
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Thu Jul 11 13:40:20 2024 +1200

    fmt fix

commit 64f2993
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Thu Jul 11 13:37:20 2024 +1200

    fmt fix

-> Skipped - merge commit <-
commit d69d2c6
Merge: f7bad22 018680b
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Thu Jul 11 13:31:19 2024 +1200

    Merge branch 'main' into ajm/tables

commit f7bad22
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Thu Jul 11 11:52:49 2024 +1200

    Test fixes for Integration tests

    ReadDocument now accepts an optional docID

    see comments in ReadAndUpdateOperation about how we create an
    upsert doc

commit adb2fa9
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Thu Jul 11 10:40:14 2024 +1200

    clean for InsertAttempt

    comments up to standard, removed unused getRow()

commit 4985a94
Author: Tatu Saloranta <tatu.saloranta@datastax.com>
Date:   Wed Jul 10 15:24:26 2024 -0700

    Replace FeatureFlags with Quarkus config setting (#1257)

-> skipped - already in main <-
commit 018680b
Author: Hazel <hazel.he@datastax.com>
Date:   Wed Jul 10 18:06:48 2024 -0400

    Follow up for PR #1251: Remove `Optional` and centralize validation (#1259)

-> SKIPPED - NOT IN MAIN BUT SAME AS 9d977cd below <-
commit bd6be18
Author: Hazel <hazel.he@datastax.com>
Date:   Wed Jul 10 16:41:24 2024 -0400

    Improve error message when EGW timeout (#1255)

-> SKIPPED - NOT IN MAIN BUT SAME AS 197bfd2 below <-
commit 33311d2
Author: Hazel <hazel.he@datastax.com>
Date:   Wed Jul 10 15:12:45 2024 -0400

    Small fix: Improve error message when not providing provider key through `x-embedding-api-key` (#1251)

-> SKIPPED - NOT IN MAIN BUT SAME AS dd652c3 below <-
commit 486977f
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Wed Jul 10 11:44:53 2024 -0400

    Use Stargate v2.1.0-BETA-13 (#1252)

    Co-authored-by: jeffreyscarpenter <12115970+jeffreyscarpenter@users.noreply.github.com>

commit 611282a
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Thu Jul 11 10:00:03 2024 +1200

    Insert working, one and many - some Test failing

-> skipped - already in main <-
commit 9d977cd
Author: Hazel <hazel.he@datastax.com>
Date:   Wed Jul 10 16:41:24 2024 -0400

    Improve error message when EGW timeout (#1255)

-> skipped - already in main <-
commit 197bfd2
Author: Hazel <hazel.he@datastax.com>
Date:   Wed Jul 10 15:12:45 2024 -0400

    Small fix: Improve error message when not providing provider key through `x-embedding-api-key` (#1251)

-> skipped - already in main <-
commit dd652c3
Author: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Date:   Wed Jul 10 11:44:53 2024 -0400

    Use Stargate v2.1.0-BETA-13 (#1252)

    Co-authored-by: jeffreyscarpenter <12115970+jeffreyscarpenter@users.noreply.github.com>

commit 7e6e99d
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Wed Jul 10 16:37:47 2024 +1200

    refactor Insert results ready for tables

commit 588b297
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Wed Jul 10 13:47:06 2024 +1200

    Refactor ReadOperationPage for reuse with tables

    and added DocumentSource

commit d62f95c
Author: Aaron Morton <aaron.morton@datastax.com>
Date:   Wed Jul 10 10:34:56 2024 +1200

    Basic findOne and find for tables

    just does a select *, in place to find re-use with collection
    commands

commit fbd09e9
Author: Tatu Saloranta <tatu.saloranta@datastax.com>
Date:   Tue Jul 9 18:08:16 2024 -0700

    Enable metadata access to "system" keyspace (for testing purposes)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants