celix icon indicating copy to clipboard operation
celix copied to clipboard

Feature/674 improve properties

Open pnoltes opened this issue 1 year ago • 6 comments
trafficstars

This PR add support for setting and getting string, bool, long, double and a version array list with properties.

Also:

  • Add support for only getting a property value if it is stored with a specific type (getLong, getDouble, etc)
  • Refactor filter to use the underlying property type when matching. This prevent the needed for memory allocation during filter match. The changed behavior should also more align with Properites and Filter matching in OSGi
  • Add logic so that a service registration will use the correct type for service.id, service.bundleid and service.version and if needed convert service.ranking and service.version properties (if set before a service registration call) to the correct types.
  • Add Properties / Filter intro documentation
  • Improve test coverage for Filter and Properties

Refactoring the Properties store and load to include properties value type information will be done in a follow up PR.

pnoltes avatar Jan 22 '24 21:01 pnoltes

~~I noticed Codecov has stopped working for 3 days in a row.~~ Fixed.

PengZheng avatar Jan 23 '24 01:01 PengZheng

Codecov Report

Attention: Patch coverage is 99.33244% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 90.17%. Comparing base (8acbe78) to head (6a9713e).

Files Patch % Lines
libs/framework/src/bundle_context.c 88.46% 3 Missing :warning:
...te_service_admin_dfi/src/import_registration_dfi.c 80.00% 1 Missing :warning:
...ervices/rsa_rpc_json/src/rsa_json_rpc_proxy_impl.c 90.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #721      +/-   ##
==========================================
+ Coverage   89.84%   90.17%   +0.32%     
==========================================
  Files         222      223       +1     
  Lines       26058    26545     +487     
==========================================
+ Hits        23413    23937     +524     
+ Misses       2645     2608      -37     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Jan 23 '24 05:01 codecov-commenter

I'll finish reviewing this PR ASAP. Please help review #699.

PengZheng avatar Jan 28 '24 15:01 PengZheng

The first round review finished. I noticed code duplications in celix_properties, and thus suggest an alternative design:

  • Add only one type to celix_properties, i.e. ArrayList
  • Add only four (or more?) methods setArrayList/assignArrayList/getArrayList/getAsArrayList
  • Move element type information into celix_array_list, so that we can have the right copy operation for them.

This way:

  • We don't need to check arrayList's element types, which is totally left to arrayList
  • The dangling pointer issue is solved naturally by the right copy operation in arrayList
  • The code duplication in celix_properties is eliminated.

WDYT?

Sounds good and I will look into this. I am bit uncertain how this impact celix_array_list and its current usage.

Currently the array list implementation has no notion a an element type and just provided access to a union element entry.
A logically way to specify the array list entry type is during creations, but this impact all usage of celix_arrayList_create and celix_arrayList_createWithOptions.

An other options could be to set the element type when entries are added, so a call to celix_arrayList_addLong will make the array list element type long and a followup call to celix_arrayList_setDouble or celix_arrayList_getDouble, etc will fail.

Or maybe mixed element types should be allowed, but I do no think this is a valid use case.

pnoltes avatar Feb 02 '24 13:02 pnoltes

The first round review finished. I noticed code duplications in celix_properties, and thus suggest an alternative design:

  • Add only one type to celix_properties, i.e. ArrayList
  • Add only four (or more?) methods setArrayList/assignArrayList/getArrayList/getAsArrayList
  • Move element type information into celix_array_list, so that we can have the right copy operation for them.

This way:

  • We don't need to check arrayList's element types, which is totally left to arrayList
  • The dangling pointer issue is solved naturally by the right copy operation in arrayList
  • The code duplication in celix_properties is eliminated.

WDYT?

Sounds good and I will look into this. I am bit uncertain how this impact celix_array_list and its current usage.

Currently the array list implementation has no notion a an element type and just provided access to a union element entry. A logically way to specify the array list entry type is during creations, but this impact all usage of celix_arrayList_create and celix_arrayList_createWithOptions.

An other options could be to set the element type when entries are added, so a call to celix_arrayList_addLong will make the array list element type long and a followup call to celix_arrayList_setDouble or celix_arrayList_getDouble, etc will fail.

Or maybe mixed element types should be allowed, but I do no think this is a valid use case.

I will create a separate pull request to add element type support to the array list, including a array list copy function. And when the array list with element type pull request is merged, I will update this PR and simplify the typed array list support for properties.

pnoltes avatar Feb 04 '24 18:02 pnoltes

This PR has been updated so that celix array list types are used, which minimizes the celix properties functions needed for array list handling. This PR can be reviewed again.

After this, I will dive into properties (JSON) serialization.

pnoltes avatar Mar 24 '24 16:03 pnoltes

All issues have been addressed! :rocket: Please wait for a little last minute cleanup to be merged, which will happen tomorrow.

PengZheng avatar Apr 01 '24 14:04 PengZheng