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

feat: server-side SDK #160

Merged
merged 80 commits into from
Oct 23, 2023
Merged

feat: server-side SDK #160

merged 80 commits into from
Oct 23, 2023

Conversation

cwaldren-ld
Copy link
Contributor

@cwaldren-ld cwaldren-ld commented Jul 6, 2023

This PR exists to verify the linting, compilation, and test CI steps for the work-in-progress Server-Side SDK.

This feature branch does not yet represent the finished product but rather an alpha. Once merged, additional work is needed to onboard to release please.

cwaldren-ld and others added 12 commits June 12, 2023 11:02
Moves the client's `ItemDescriptor` to internal where it can be shared,
and gives it a generic parameter so it can hold flag rules/segments.
Augments event processor with context-key deduplication abilities using
an LRU cache.
This PR extends our existing deserialization/serialization technique
from the client-side SDK to the server-side SDK data model.

Specifically, it adds deserialization for the segment structure. I've
also introduced some helpers to make the logic less repetitive, and
centralize some decisions (like what to do if a JSON field isn't
present, or is null.)

As background, the `tag_invoke` functions present in this PR are what
allows the `boost::json` library to recursively deserialize complex
structures.

Here, I've added a couple new `tag_invokes` for bool, string, vector,
etc. The special part is that they deserialize into `expected<type,
error>` rather than the primitive directly; this allows us to return an
error if the deserialization failed.
This PR builds on #153, extending the deserialization to flag data. 

I've added to the `PARSE_FIELD` macro to handle the following cases:
1. Parse a field where the zero-value is a valid part of the domain:
`PARSE_FIELD`. In other words, if the field is omitted or null, it will
be filled in with a default value. Most common scenario.
2. Like (1), but supply a default value in case the field is null or
omitted: `PARSE_FIELD_DEFAULT`.
3. Parse a field that doesn't have a valid default value, and so is
represented as an `optional`: `PARSE_CONDITIONAL_FIELD`. For example,
the value `""` wouldn't be a valid default for a unique key.
4. Parse a field that must be present in the JSON data:
`PARSE_REQUIRED_FIELD`. Usage of this should be rare; the test would be
if the SDK is totally unable to function without this.

To achieve this, I've made the `tag_invoke`s operate on
`expected<optional<T>, JsonError`'s, instead of `expected<T, JsonError`.

We could potentially revert that (and instead add a new enum case to
`JsonError`), but I figure `optional` is a good representation of
null/omitted. I think I need to actually try removing it to determine if
it results in a better API.

One thing lacking is better error messages - we use kSchemaFailure
everywhere but we now have the ability to report exactly what was
missing/wrong type, etc.
Adds in missing `Flag` model to the top-level `SDKDataSet`.

Additionally, both the flag/segment fields now use use `PARSE_FIELD`, so
we can get an empty `unordered_map` if they are missing.
@cwaldren-ld cwaldren-ld marked this pull request as ready for review July 12, 2023 22:55
@cwaldren-ld cwaldren-ld requested a review from a team July 12, 2023 22:55
@cwaldren-ld cwaldren-ld marked this pull request as draft July 12, 2023 22:55
kinyoklion and others added 15 commits July 12, 2023 15:56
Previously, context kind was an `std::string`. 

The requirement that it not be an empty string was enforced by parent
data model deserializers.

Now, we have a dedicated type-safe wrapper via`BOOST_STRONG_TYPEDEF`.
The custom `tag_invoke` enforces the invariant so it's not possible to
deserialize it inconsistently.
This commit contains the server-side evaluation engine and associated tests.
Current API of `EvaluationStack` is very easy to accidentally misuse
because it takes a reference to its key argument. If the arg is
destroyed, then we'll have a use-after-free.
Also fixes some style / const-correct issues that I found while
documenting.
Adds a build for the server SDK, with various build fixes.
Adds a hello app for the Server-side SDK.

Modifies the existing apps to fix CMake target name conflicts.
public:
using DateTime = std::chrono::time_point<std::chrono::system_clock>;

/**
Copy link
Contributor Author

@cwaldren-ld cwaldren-ld Oct 20, 2023

Choose a reason for hiding this comment

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

Calling out as a theoretical breaking change.

In previous Client SDK, you could call IDataSourceStatusProvider::Status() and get back a class client_side::data_sources::DataSourceStatus.

Now, you get back a class common::data_sources::DataSourceStatusBase<DataSourceState>.

These classes should have the same API, but the concrete type has changed.

If the caller did:

auto status = sdk.DataSourceStatus().Status();
if (auto err = status.LastError()) {
   ...
}

then this should compile without any changes. But if they did:

client_side::data_sources::DataSourceStatus status = sdk.DataSourceStatus().Status();

that's going to require a change.

We have some precedent for "non-major to fix a bug in the API definition" - but not sure if this counts.

Copy link
Contributor Author

@cwaldren-ld cwaldren-ld Oct 20, 2023

Choose a reason for hiding this comment

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

Actually, this might be fine. The client_side::data_sources::DataSourceStatus is still present in the type system, it's just an alias for the common class.

It's (probably?) ABI breaking but we don't claim to support a stable C++ ABI.

Copy link
Contributor Author

@cwaldren-ld cwaldren-ld Oct 20, 2023

Choose a reason for hiding this comment

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

The following code compiles without issue on the existing client, and the client in this PR:

  client_side::data_sources::DataSourceStatus ds_status =
        client.DataSourceStatus().Status();

    std::optional<client_side::data_sources::DataSourceStatus::ErrorInfo>
        last_err = ds_status.LastError();

    client_side::data_sources::DataSourceStatus::DataSourceState state =
        ds_status.State();

    client_side::data_sources::DataSourceStatus::DateTime date =
        ds_status.StateSince();

@cwaldren-ld cwaldren-ld marked this pull request as ready for review October 20, 2023 21:33
@cwaldren-ld cwaldren-ld requested a review from a team October 20, 2023 21:33
@cwaldren-ld
Copy link
Contributor Author

cwaldren-ld commented Oct 20, 2023

I've reviewed all files, fixed a bunch of outstanding TODOs, and got this branch all caught up with main.

I'm feeling ready to merge pending a backwards compatibility concern I've called out in comments. (think I got myself confused on that one.)

@cwaldren-ld cwaldren-ld enabled auto-merge (squash) October 23, 2023 17:49
@cwaldren-ld cwaldren-ld merged commit 75eece3 into main Oct 23, 2023
13 of 14 checks passed
@cwaldren-ld cwaldren-ld deleted the server-side branch October 23, 2023 17:49
@github-actions github-actions bot mentioned this pull request Oct 23, 2023
cwaldren-ld pushed a commit that referenced this pull request Oct 23, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>launchdarkly-cpp-client: 3.2.0</summary>

##
[3.2.0](launchdarkly-cpp-client-v3.1.1...launchdarkly-cpp-client-v3.2.0)
(2023-10-23)


### Features

* server-side SDK
([#160](#160))
([75eece3](75eece3))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * launchdarkly-cpp-internal bumped from 0.2.0 to 0.3.0
    * launchdarkly-cpp-common bumped from 0.4.0 to 0.5.0
</details>

<details><summary>launchdarkly-cpp-common: 0.5.0</summary>

##
[0.5.0](launchdarkly-cpp-common-v0.4.0...launchdarkly-cpp-common-v0.5.0)
(2023-10-23)


### Features

* server-side SDK
([#160](#160))
([75eece3](75eece3))
</details>

<details><summary>launchdarkly-cpp-internal: 0.3.0</summary>

##
[0.3.0](launchdarkly-cpp-internal-v0.2.0...launchdarkly-cpp-internal-v0.3.0)
(2023-10-23)


### Features

* server-side SDK
([#160](#160))
([75eece3](75eece3))


### Dependencies

* The following workspace dependencies were updated
  * dependencies
    * launchdarkly-cpp-common bumped from 0.4.0 to 0.5.0
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This was referenced Oct 23, 2023
cwaldren-ld pushed a commit that referenced this pull request Oct 25, 2023
🤖 I have created a release *beep* *boop*
---


<details><summary>launchdarkly-cpp-server: 0.1.0</summary>

## 0.1.0 (2023-10-25)


### Features

* server-side SDK
([#160](#160))
([75eece3](75eece3))


### Bug Fixes

* allow for installing only the client or server SDK independently
([#269](#269))
([fe08c3c](fe08c3c))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants