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.

Sign up for GitHub

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

Jump to bottom

Feature/685 properties json serialization #743

Merged
merged 27 commits into from May 24, 2024
Merged

Feature/685 properties json serialization #743

merged 27 commits into from May 24, 2024

Conversation

pnoltes
Copy link
Contributor

@pnoltes pnoltes commented Apr 16, 2024

This PR introduces JSON encoding for properties using the libjansson library, thereby making libjansson a mandatory dependency for the utils library and, consequently, for the framework library as well.

This PR does not implement the use of the added JSON encoding for properties. This is intentional, so that the pull request review can focus how property JSON encoding should function. Since the JSON encoding is not yet used, the previous properties encoding functions are still in place. Consequently, some of the JSON encoding functions have been temporarily suffixed with "2" (e.g., celix_properties_load2). These functions will be renamed before a release of Apache Celix 3.0.0.

The usage of JSON encoding for configuration properties and bundle manifests will be done in one or two follow-up pull requests.

Note that although the JSON encoding for properties is not yet used for configuration properties and bundle manifests, it can already be used elsewhere (e.g., in the remote event admin) once this PR is merged.

@pnoltes
gh-685: Initial properties save/load setup
3acdb74
@pnoltes
gh-685: Add jansson dep for simple properties save/load
9511610
@pnoltes
gh-685: Add json prop loading for primitive and initial arr.
de07256
@pnoltes
gh-685: Add support for nested obj properties decoding
04bccbd
@pnoltes
gh-685: Rename prop serialization sources to prop encoding
77cd6b3
@pnoltes
gh-685: Add initial setup for properties decode flags
72c785d
@pnoltes
gh-685: Add support for a flat and nested flag for prop encoding
8e03a82
@pnoltes
gh-685: Add additional prop encoding test based on test cov
0ceaeb8
@pnoltes
gh-685: Add error injection test for properties encoding
99e503a
@pnoltes
gh-685: Separate version ei testing to prevent own ei code inteference
56c6767
@pnoltes
gh-685: Refactor assert to prevent used var
ac2b131 
Also: Rename the nested / flat encoding style flag.
@pnoltes
gh-685: Fix several memleaks in properties encoding
0e58944
@pnoltes
gh-685: Fix incorrect celix_autoptr usage
ae4464c
@pnoltes
gh-685: Fix incorrect celix_autoptr usage
a4814f3
@pnoltes
gh-685: Add properties encoding documentation
aa29987
@pnoltes
gh-685: Some whitespace corrections
c46a483
@xuzhenbao xuzhenbao mentioned this pull request Apr 17, 2024
Closed
@PengZheng
Copy link
Contributor

Just begin reviewing this PR. Sorry for being late.

PengZheng
PengZheng requested changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

I just reviewed the interface design and the encoding implementation and spotted several issues.

libs/utils/include/celix_properties.h Outdated Show resolved Hide resolved
libs/utils/include/celix_properties.h Outdated Show resolved Hide resolved
libs/utils/include/celix_properties.h Outdated Show resolved Hide resolved
libs/utils/include/celix_properties.h Outdated Show resolved Hide resolved
libs/utils/include/celix_properties.h Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
PengZheng
PengZheng requested changes Apr 28, 2024
View reviewed changes
Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

My first round review is finished, and I have left some additional remarks on decoding implementation.

libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
libs/utils/src/properties_encoding.c Outdated Show resolved Hide resolved
@pnoltes
Copy link
Contributor Author

pnoltes commented Apr 28, 2024

@PengZheng Thanks for the thorough review.
I will not be able to rework this PR the coming, but I will pick this up somewhere next week.

@PengZheng
Copy link
Contributor

I will not be able to rework this PR the coming, but I will pick this up somewhere next week.

No problem. 😄

@pnoltes
Copy link
Contributor Author

pnoltes commented May 20, 2024

I have processed al review comments.
The brew MacOS ci test is failing, but I think this is an issue not introduced by this PR. I will try to have a look at this later, but for now this PR can be re-reviewed.

@pnoltes pnoltes requested a review from PengZheng May 20, 2024 20:42
@PengZheng
Copy link
Contributor

The brew MacOS ci test is failing, but I think this is an issue not introduced by this PR. I will try to have a look at this later, but for now this PR can be re-reviewed.

It has been addressed by #745

PengZheng
PengZheng reviewed May 22, 2024
View reviewed changes
Copy link
Contributor

@PengZheng PengZheng left a comment

Choose a reason for hiding this comment

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

It seems all major issues have been addressed.
I will add some minor cleanup before merging.

libs/utils/include/celix_properties.h Outdated Show resolved Hide resolved
PengZheng
PengZheng approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@PengZheng PengZheng left a comment •
edited

Choose a reason for hiding this comment

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

Improve corner case treatment, check TEST_F(PropertiesSerializationTestSuite, SavePropertiesWithArrayListsContainingNaNAndInfValueTest) for more detail.

There are still two issues for C++ API left:
#743 (comment)
#743 (comment)

I have no other complaints. Let's get this nice enhancement merged.

@PengZheng PengZheng merged commit e4df2aa into master May 24, 2024
32 checks passed
@PengZheng PengZheng deleted the feature/685-properties-json-serialization branch May 24, 2024 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xuzhenbao xuzhenbao xuzhenbao left review comments

@PengZheng PengZheng PengZheng approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pnoltes @PengZheng @xuzhenbao