type support for properties #470

pnoltes · 2023-01-12T10:50:08Z

This PR add some type support to celix_properties_t. Type support for properties are added so that #432 can be solved in type-aware manner.

Looking at the LDAP spec, the filter attribute type should lead to the correct order comparison.
And because in Celix a "service.version" properties is used to identify the runtime version of a service, it should be possible to add a "service.version" property value as celix_version_t type so that celix_version_compareTo can be used to compare "celix.version" attributes.

For now I want to keep this PR draft, for 2 reasons:

In this PR, celix properties are no longer a typedef of hash_map, and this is a backwards incompatible update. IMO this PR should only be merged if the next Celix release is going to be a 3.0.0 release
Concerning Filter version comparison #432, I think it is better to first solve this differently. Akin what @tira-misu purposes in the issue. Although I think adding some type support for properties is eventually a better solution (especially considering performance), for now a solution that does not require a major version update is preferred.

But comments are welcome :)

Overview changes:

Add missing doxygen for celix_properties.h
Reimplement celix_properties_t using celix_string_hash_map_t instead of hashmap
Add support for long, double, bool and celix_version_t types for property values
Add a "short properties" optimization, so that most service properties only need a single malloc.
Move and update version cpputest to gtest
Move and update properties cpputest to gtest
Add C++ Version wrapper for the celix_version_t
Replace usage of hashMapIterator on properties and CELIX_PROPERTIES_FOR_EACH to CELIX_PROPERTIES_ITERATE

…sh map.

…ITERATE

…t_for_properties # Conflicts: # bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/src/pubsub_wire_protocol_common.c

PengZheng · 2023-01-12T10:53:13Z

Nice! I'll have a detailed look at this.

codecov-commenter · 2023-01-12T11:01:07Z

Codecov Report

Attention: 57 lines in your changes are missing coverage. Please review.

Comparison is base (25463cf) 81.65% compared to head (929ed1e) 83.12%.

❗ Current head 929ed1e differs from pull request most recent head 95460e9. Consider uploading reports for the commit 95460e9 to get more accurate results

Files	Patch %	Lines
libs/utils/src/properties.c	94.93%	24 Missing ⚠️
libs/framework/src/service_reference.c	0.00%	13 Missing ⚠️
libs/utils/src/celix_hash_map.c	96.77%	7 Missing ⚠️
...ubsub_admin_websocket/src/pubsub_websocket_admin.c	0.00%	4 Missing ⚠️
libs/utils/include/celix/Properties.h	95.78%	4 Missing ⚠️
...e_service_admin_dfi/src/remote_service_admin_dfi.c	50.00%	3 Missing ⚠️
...e_services/topology_manager/src/topology_manager.c	0.00%	1 Missing ⚠️
libs/utils/src/utils.c	83.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #470      +/-   ##
==========================================
+ Coverage   81.65%   83.12%   +1.46%     
==========================================
  Files         252      254       +2     
  Lines       32559    32935     +376     
==========================================
+ Hits        26587    27377     +790     
+ Misses       5972     5558     -414

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pnoltes · 2023-02-06T12:13:17Z

When Feature/refactor bundle cache #476 is merged, update this PR to use the celix utils string convert to functions

pnoltes · 2023-04-09T15:04:15Z

Note because this will be merged when Celix 3.0.0 is prepared also:

Update the put functions for celix string/long hash map with a celix_status_t return so that the return value can be used to indicate a CELIX_ENOMEN error. Note that currently the previous stored value is returned.
Update the celix properties set functions with a celix_status_t return so that the return value can be used to indicate a CELIX_ENOMEM error
Add celix ei functions for celix string/long hash map put functions and celix properties set functions

…t_for_properties # Conflicts: # libs/framework/include/celix/dm/DependencyManager_Impl.h # libs/framework/src/celix_launcher.c # libs/framework/src/service_reference.c # libs/utils/CMakeLists.txt # libs/utils/gtest/CMakeLists.txt # libs/utils/include/celix/Properties.h # libs/utils/include/celix_properties.h # libs/utils/include/celix_version.h # libs/utils/private/test/version_test.cpp # libs/utils/src/properties.c # libs/utils/src/version.c

PengZheng

An interface for testing (in)equality will be very handy.

PengZheng

How about we provide a unified way of serialization (e.g. netstring) so that it can be used for both file storage and EventAdmin?

pnoltes · 2023-05-31T17:41:56Z

How about we provide a unified way of serialization (e.g. netstring) so that it can be used for both file storage and EventAdmin?

I agree and I will also look into this when we start working on Celix 3.0.0. I also think we should (de)serialize with type information.

…t_for_properties # Conflicts: # libs/utils/CMakeLists.txt # libs/utils/gtest/CMakeLists.txt # libs/utils/gtest/src/HashMapTestSuite.cc # libs/utils/include/celix/Exception.h # libs/utils/include/celix_properties.h # libs/utils/include/celix_utils.h # libs/utils/include/celix_version.h # libs/utils/private/test/properties_test.cpp # libs/utils/src/properties.c

…d on benchmark output Also add internal header for long/string hash map and properties so that hash map statistics can be extracted - in the project - for analysing purposes.

…he/celix into feature/type_support_for_properties

Also add some addition celix properties statistics.

libs/utils/include/celix_long_hash_map.h

libs/utils/src/celix_hash_map.c

Also add/improve some additional error handling in properties.

libs/utils/src/properties.c

…_fillEntry shorter.

libs/utils/src/properties.c

Also improve properties error handling

pnoltes · 2023-11-18T14:05:33Z

I agree that "the index of the end iterator is the same as the size of the map".
The current implementation of celix_stringHashMapIterator_next allows iter->index > map->genericMap.size. Is it intended?

Ah i see, no this was not intentionally and I think it is better to keep the max iterator index on map size. Refactor the index increment for this.

pnoltes · 2023-11-18T15:53:24Z

@PengZheng I think I reworked all the remarks.

Could you re-review?

PengZheng · 2023-11-19T05:36:56Z

Ah I see, no this was not intentionally and I think it is better to keep the max iterator index on map size. Refactor the index increment for this.

In the original implementation, index has a well-defined meaning: the entry pointed by the iterator is the index-th entry in this iteration. Without this meaning, the newly added test TEST_F(HashMapTestSuite, IterateWithRemoveAtIndex4Test) seems weird:

    celix_autoptr(celix_string_hash_map_t) sMap = createStringHashMap(6);
    auto iter1 = celix_stringHashMap_begin(sMap);
    while (!celix_stringHashMapIterator_isEnd(&iter1)) {
        if (iter1.index == 4) {
            // note only removing entries where the iter key is even
            celix_stringHashMapIterator_remove(&iter1);
        } else {
            celix_stringHashMapIterator_next(&iter1);
        }
    }
    EXPECT_EQ(4, celix_stringHashMap_size(sMap));

4 rather than 5 entries remain. The original implementation and well-defined meaning seems better. Sorry for misguiding you.

index is arguably an external concept, since we don't rely on it to decide whether the current iteration ends:

bool celix_stringHashMapIterator_isEnd(const celix_string_hash_map_iterator_t* iter) {
    return iter->_internal[1] == NULL; //check if entry is NULL
}

bool celix_longHashMapIterator_isEnd(const celix_long_hash_map_iterator_t* iter) {
    return iter->_internal[1] == NULL; //check if entry is NULL
}

Therefore, it is also reasonable to remove index from the interface and implementation.
WDYT?

PengZheng

The only major issue left is the iterator index as mentioned above.
I apologize for the misguidance caused by misunderstanding.

Otherwise, this PR can be merged except for 2 minor issues:
#470 (review)

…erties_storeEscapedString.

pnoltes · 2023-11-20T17:51:08Z

Therefore, it is also reasonable to remove index from the interface and implementation. WDYT?

I agree. The index field is too confusion and it is trivial to add a counter/index on your own, so I removed the index field.

PengZheng

Nice work. Now we have a more robutst, more efficient, and well-documented celix_properties_t.

LGTM

pnoltes added 14 commits December 31, 2022 16:58

Rename current properties imp to properties_v1.c

c28810e

Refactor celix properties to be based on celix_string_hash_map.

1ff603b

Add initial primitive type support for celix properties entries

6156c70

Improve celix properties iterator

02b08a1

Add unit test for celix types properties and fix some mem issues

2c5194f

Add properties type support to C++ properties

740bbbf

Improve test coverage and fix issues in properties and string/long ha…

efa1017

…sh map.

Refactor and improve properties and string hash map test suite and impl

ced3279

Remove old properties.c file

71daacf

Fix Properties implementation for C++11 and C++14.

efb74c1

Improve codecov and replace usage of CELIX_PROPERTIES_FOR_EACH with _…

4f86a3d

…ITERATE

Adjust short properties optimization values based on some basic testing

c15bf71

Merge remote-tracking branch 'origin/master' into feature/type_suppor…

1ce505d

…t_for_properties # Conflicts: # bundles/pubsub/pubsub_protocol/pubsub_protocol_lib/src/pubsub_wire_protocol_common.c

Fix issue with setting properties with NULL values

fe434a0

pnoltes marked this pull request as draft January 12, 2023 10:50

pnoltes mentioned this pull request Apr 6, 2023

Prepare Celix 3.0.0 release #509

Open

42 tasks

This was referenced Apr 9, 2023

Refactor last cpputests with google tests #516

Open

EventAdmin #515

Open

pnoltes added 4 commits May 7, 2023 15:19

Refactor properties and version after merge with master

0c0bf14

Refactor cpputest version ei test to gtest

78b94d5

Add missing output param in serviceReference_getPropertyKeys

717869d

PengZheng reviewed May 30, 2023

View reviewed changes

pnoltes added 3 commits November 12, 2023 13:26

Refactor hash map long hash, load factor and hash function usage base…

3964e0b

…d on benchmark output Also add internal header for long/string hash map and properties so that hash map statistics can be extracted - in the project - for analysing purposes.

Merge branch 'feature/type_support_for_properties' of github.com:apac…

468a161

…he/celix into feature/type_support_for_properties

Fix memleak in hash map by first resizing and then adding entry

10e16f6

Also add some addition celix properties statistics.

PengZheng reviewed Nov 13, 2023

View reviewed changes

libs/utils/include/celix_long_hash_map.h Outdated Show resolved Hide resolved

libs/utils/src/celix_hash_map.c Outdated Show resolved Hide resolved

libs/utils/src/celix_hash_map.c Outdated Show resolved Hide resolved

PengZheng and others added 2 commits November 13, 2023 21:29

Update hash map maxLoadFactor documentation.

8248da5

Refactor celix_properties fill/create entry

d1206d9

Also add/improve some additional error handling in properties.

PengZheng reviewed Nov 18, 2023

View reviewed changes

libs/utils/src/properties.c Outdated Show resolved Hide resolved

Add an extra byte for the terminating null byte to the value buffer.

038808e

PengZheng reviewed Nov 18, 2023

View reviewed changes

libs/utils/src/properties.c Outdated Show resolved Hide resolved

PengZheng added 2 commits November 18, 2023 13:22

Detect error condition in string formatting.

804d633

Detect error condition in string formatting and make celix_properties…

74f9550

…_fillEntry shorter.

PengZheng reviewed Nov 18, 2023

View reviewed changes

libs/utils/src/properties.c Outdated Show resolved Hide resolved

PengZheng reviewed Nov 18, 2023

View reviewed changes

libs/utils/src/properties.c Outdated Show resolved Hide resolved

PengZheng and others added 3 commits November 18, 2023 16:02

Fix initial hash table capacity in celix_properties.

3ff7046

Fix misplaced documentation and update it.

1706890

Refactor celix_properties_size return type to size_t

4d4e9f7

Also improve properties error handling

pnoltes added 2 commits November 18, 2023 15:19

Fix hash map iterator index tests and behaviour

492c457

Refactor properties createEntry to use destroyEntry

03731d6

pnoltes requested a review from PengZheng November 18, 2023 15:53

PengZheng requested changes Nov 19, 2023

View reviewed changes

PengZheng and others added 2 commits November 19, 2023 14:48

Avoid unnecessary strlen call and check error condition in celix_prop…

caedd79

…erties_storeEscapedString.

Remove index field from hashmap and properties iterator

78d7879

Add ftell check and error injection test for properties

95460e9

PengZheng approved these changes Nov 21, 2023

View reviewed changes

pnoltes merged commit e9642b9 into master Nov 21, 2023
28 checks passed

pnoltes deleted the feature/type_support_for_properties branch November 21, 2023 07:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

type support for properties #470

type support for properties #470

pnoltes commented Jan 12, 2023 •

edited

PengZheng commented Jan 12, 2023

codecov-commenter commented Jan 12, 2023 •

edited

pnoltes commented Feb 6, 2023 •

edited

pnoltes commented Apr 9, 2023 •

edited

PengZheng left a comment

PengZheng left a comment •

edited

pnoltes commented May 31, 2023

pnoltes commented Nov 18, 2023

pnoltes commented Nov 18, 2023 •

edited

PengZheng commented Nov 19, 2023 •

edited

PengZheng left a comment •

edited

pnoltes commented Nov 20, 2023

PengZheng left a comment

type support for properties #470

type support for properties #470

Conversation

pnoltes commented Jan 12, 2023 • edited

PengZheng commented Jan 12, 2023

codecov-commenter commented Jan 12, 2023 • edited

Codecov Report

pnoltes commented Feb 6, 2023 • edited

pnoltes commented Apr 9, 2023 • edited

PengZheng left a comment

Choose a reason for hiding this comment

PengZheng left a comment • edited

Choose a reason for hiding this comment

pnoltes commented May 31, 2023

pnoltes commented Nov 18, 2023

pnoltes commented Nov 18, 2023 • edited

PengZheng commented Nov 19, 2023 • edited

PengZheng left a comment • edited

Choose a reason for hiding this comment

pnoltes commented Nov 20, 2023

PengZheng left a comment

Choose a reason for hiding this comment

pnoltes commented Jan 12, 2023 •

edited

codecov-commenter commented Jan 12, 2023 •

edited

pnoltes commented Feb 6, 2023 •

edited

pnoltes commented Apr 9, 2023 •

edited

PengZheng left a comment •

edited

pnoltes commented Nov 18, 2023 •

edited

PengZheng commented Nov 19, 2023 •

edited

PengZheng left a comment •

edited