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

Make our JSON APIs a public interface sd-json.h #32628

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

poettering
Copy link
Member

@poettering poettering commented May 2, 2024

This is preparation for making our Varlink API a public API. Since our Varlink API is built on top of our JSON API we need to make that public first (it's a nice API, but JSON APIs there are already enough, this is purely about the Varlink angle).

@poettering
Copy link
Member Author

This is v257 material, in case this wasn't clear.

Copy link

github-actions bot commented May 2, 2024

Important

An -rc1 tag has been created and a release is being prepared, so please note that PRs introducing new features and APIs will be held back until the new version has been released.

@yuwata yuwata added the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label May 2, 2024
@github-actions github-actions bot removed the ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR label May 2, 2024
@bluca bluca added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed please-review PR is ready for (re-)review by a maintainer labels May 3, 2024
@bluca
Copy link
Member

bluca commented May 3, 2024

Code coverage at 81% is good, but there's a few spots missing that looks like should be covered: json_parse_with_source_continue() json_dispatch_int8(), and the _JSON_BUILD_VARIANT_ARRAY, _JSON_BUILD_STRING_SET, _JSON_BUILD_CALLBACK cases in json_buildv()

Could you please add unit tests to cover these? Then test-wise we should be in a good spot.

https://coveralls.io/builds/67280970/source?filename=src%2Fshared%2Fjson.c#L3354

@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels May 6, 2024
@poettering
Copy link
Member Author

Could you please add unit tests to cover these? Then test-wise we should be in a good spot.

Sure, here you go

@bluca
Copy link
Member

bluca commented May 6, 2024

Could you please add unit tests to cover these? Then test-wise we should be in a good spot.

Sure, here you go

nice - mind splitting that out in a separate PR? It can be merged immediately then

@poettering
Copy link
Member Author

nice - mind splitting that out in a separate PR? It can be merged immediately then

too lazy for that. all the symbols changed names.

@bluca bluca added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR and removed please-review PR is ready for (re-)review by a maintainer labels May 6, 2024
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels May 6, 2024
The functions more or less do the same thing. Merge them.

This makes json_dispatch_path() the common resulting implementation. it
learnt:

1. Will reset the path to NULL if specified as null in JSON
2. Depending on the JSON_SAFE flag will insist on normalized path or not

With this the two implementations are identical, except for the
differences now toggable via JSON_SAFE flag
This is preparation for making our Varlink API a public API. Since our
Varlink API is built on top of our JSON API we need to make that public
first (it's a nice API, but JSON APIs there are already enough, this is
purely about the Varlink angle).

I made most of the json.h APIs public, and just placed them in
sd-json.h. Sometimes I wasn't so sure however, since the underlying data
structures would have to be made public too. If in doubt I didn#t risk
it, and moved the relevant API to src/libsystemd/sd-json/json-util.h
instead (without any sd_* symbol prefixes).

This is mostly a giant search/replace patch.
The flag is fairly generic these days and just selects a slightly
stricter validation, with details depending on the selected dispatch
function. Hence, let's give it more precise name, in particular one that
mirrors the SD_JSON_RELAXED flag nicely (which does the opposite:
relaxes parsing)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment