Refactor cfstore to dataclasses #124

jacomago · 2025-11-04T11:25:43Z

Large refactor of cfstore

Adds dataclasses to handle all the intemediary types rather than the usage of dictionaries for everything.
Swaps to snake case (where not using Twisted stuff)
Adds typing everywhere I can
Adds pydoc to every method
Splits up the bigger methods to be much smaller
Adds the full data structures to the log messages
Cleans up exception messages to include more debug information

simon-ess

Overall this is a clear improvement!

server/recceiver/cfstore.py

simon-ess · 2025-11-05T09:31:46Z

server/recceiver/cfstore.py

                            cf_channel["properties"].append(
                                create_recordType_property(
-                                    owner, iocs[channels_dict[cf_channel["name"]][-1]]["recordType"]
+                                    ioc_info.owner, iocs[channels_iocs[cf_channel["name"]][-1]]["recordType"]


Should this be a .recordType here? We use .owner above on the same object...

(These objects are very confusing to follow... this is clearly going to be an improvement but ugh)

server/recceiver/cfstore.py

anderslindho · 2025-11-05T14:25:01Z

server/recceiver/cfstore.py

+                                    alias_channel["properties"] = __merge_property_lists(
                                        [
                                            create_active_property(owner),
                                            create_time_property(owner, iocTime),
                                        ],
-                                        alias,
+                                        alias_channel,
                                        processor.managed_properties,
                                    )


Why not do this in the above row when you're declaring the dict? 🔧

server/recceiver/cfstore.py

anderslindho · 2025-11-05T14:36:07Z

server/recceiver/cfstore.py


    if iocid not in iocs:
-        _log.warning("IOC Env Info not found: {iocid}".format(iocid=iocid))
+        _log.warning("IOC Env Info %s not found in ioc list: %s", ioc_info, iocs)


Can we please improve this msg/phrasing while we're at it? I hate "IOC Env Info"

server/recceiver/cfstore.py

anderslindho · 2025-11-05T14:40:43Z

server/recceiver/cfstore.py

+):
+    for cf_channel in old_channels:
+        if (
+            len(new_channels) == 0 or cf_channel.name in records_to_delete


No reason not to do not new_channels or ...

Still seems to apply

server/recceiver/cfstore.py

anderslindho

I'll end the review here, as it looks unfinished from 2add230

server/recceiver/cfstore.py

In favour of create_property_blah

log the whole object when available use inbuilt log formatting rather than f strings or .format (should improve performance)

anderslindho

It's pretty good, but IMO we should be fixing these remaining idiomatic python/PEP-8 issues before merge. Can all be in a single new commit if you want, TBH.

anderslindho · 2025-12-05T07:13:39Z

server/recceiver/cfstore.py

+    Active = enum.auto()
+    Inactive = enum.auto()


Thinking more about this, StrEnum is def the way to go - don't think we have any other option for CFPropertyName.

anderslindho · 2025-12-05T07:14:33Z

server/recceiver/cfstore.py

 from . import interfaces
+from .interfaces import CommitTransaction
+from .processors import ConfigAdapter


We should prob convert the project to use abs imports - could be done together with splitting up cfstore.py 💭

anderslindho · 2025-12-05T07:15:36Z

server/recceiver/cfstore.py

+    cf_query_limit: int = DEFAULT_QUERY_LIMIT
+
+    @classmethod
+    def loads(cls, conf: ConfigAdapter) -> "CFConfig":


loads usually means "load from string" (and then load is typically from file), but I guess this is OK because from_configadapter would be a bit odd/is not immediately clear (that class is a bit strange TBH)

I do agree that I expect loads to be from a string, but I would otherwise interpret load as just kind of generic. So maybe it would be better in this case?

server/recceiver/cfstore.py

jacomago · 2025-12-05T14:50:53Z

@anderslindho I dropped the override commit, only supported on 3.12 and above.

simon-ess · 2025-12-05T15:03:11Z

server/recceiver/cfstore.py

+    cf_query_limit: int = DEFAULT_QUERY_LIMIT
+
+    @classmethod
+    def loads(cls, conf: ConfigAdapter) -> "CFConfig":


I do agree that I expect loads to be from a string, but I would otherwise interpret load as just kind of generic. So maybe it would be better in this case?

simon-ess · 2025-12-05T15:07:27Z

server/recceiver/cfstore.py

+    def from_dict(cls, prop_dict: Dict[str, str]) -> "CFProperty":
+        """Create CFProperty from Channelfinder json output.
+
+        Args:
+            prop_dict: Dictionary representing a property from Channelfinder.
+        """
+        return cls(
+            name=prop_dict.get("name", ""),
+            owner=prop_dict.get("owner", ""),
+            value=prop_dict.get("value"),
+        )


Do you really need this function? You can also allow defaults in the class definition and just do

CFProperty(**some_dict)

Its the frustrating 'channels' field in the prop_dict :( that blocks doing it that way.

simon-ess · 2025-12-05T15:08:33Z

server/recceiver/cfstore.py

+    @classmethod
+    def from_dict(cls, channel_dict: Dict[str, Any]) -> "CFChannel":
+        """Create CFChannel from Channelfinder json output.
+
+        Args:
+            channel_dict: Dictionary representing a channel from Channelfinder.
+        """
+        return cls(
+            name=channel_dict.get("name", ""),
+            owner=channel_dict.get("owner", ""),
+            properties=[CFProperty.from_dict(p) for p in channel_dict.get("properties", [])],
+        )


Same question as above.

simon-ess · 2025-12-05T15:15:39Z

server/recceiver/cfstore.py

+                    CFPropertyName.HOSTNAME.value,
+                    CFPropertyName.IOC_NAME.value,
+                    CFPropertyName.IOC_ID.value,
+                    CFPropertyName.IOC_IP.value,
+                    CFPropertyName.PV_STATUS.value,
+                    CFPropertyName.TIME.value,
+                    CFPropertyName.RECCEIVER_ID.value,


Do we actually need the .values here? Can we not just work with the enum entities?

This is kind of a general question tbh - everywhere I saw the enums used they were used as their .value version. Can't they be let be except when you are printing them?

CFPropertyName is a tricky case as you mentioned, in that this is the 'default' property names. You can have them for the env variables and the info tags.

tynanford · 2025-12-06T03:15:28Z

as the resident ancient python supporter, dataclasses are >= 3.7 and are backported to 3.6 via the pip package. That is fine with me but I thought it is worth mentioning for the 3.6 crowd

I think this is an interesting topic, @tynanford: should we continue to support EOL python versions for our new releases?

Personally I think there is value supporting 3.6 for the user community. It is EOL but it is the default python version on redhat and the RHEL variants. Redhat backports security fixes to 3.6 and will until 2029. Not saying we need to continue 3.6 support until then but I also don't see a big reason to drop it now

sonarqubecloud · 2025-12-08T12:25:53Z

Quality Gate passed

Issues
16 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

jacomago · 2025-12-08T12:47:46Z

@anderslindho name to value in https://github.com/ChannelFinder/recsync/compare/26d6654a5786ff342ad5928093d30ab2e37cd0ab..f74749eb22ba3c1f833fb382dbd903f3f8c7b865 is the only change

jacomago self-assigned this Nov 4, 2025

jacomago force-pushed the refactor_cfstore_to_dataclasses branch 2 times, most recently from a303d96 to 5a4b537 Compare November 4, 2025 15:24

simon-ess reviewed Nov 5, 2025

View reviewed changes

anderslindho reviewed Nov 5, 2025

View reviewed changes

jacomago force-pushed the refactor_cfstore_to_dataclasses branch 2 times, most recently from 5c64534 to 2c90757 Compare November 6, 2025 09:35

jacomago mentioned this pull request Nov 13, 2025

Refactor docker ioc used in testing recceiver #127

Merged

jacomago added 2 commits November 27, 2025 16:12

Use a dataclass for CFConfig

2902ca5

Use set for cf_properties

87c8196

jacomago force-pushed the refactor_cfstore_to_dataclasses branch from 2c90757 to 2add230 Compare November 27, 2025 15:12

anderslindho reviewed Nov 27, 2025

View reviewed changes

jacomago force-pushed the refactor_cfstore_to_dataclasses branch 3 times, most recently from 2a666e8 to 3375756 Compare December 1, 2025 14:52

jacomago added 5 commits December 1, 2025 16:10

Use dataclass for CFProperty

78a72ac

Add committransaction dataclass

7ebeaff

Add RecordInfo dataclass

981fdab

Add CFPropertyName enum

56bacf7

Add PVStatus enum

d9b1c0c

jacomago force-pushed the refactor_cfstore_to_dataclasses branch 2 times, most recently from 012aae6 to 919b9fa Compare December 1, 2025 15:34

jacomago added 7 commits December 2, 2025 13:13

Create IocInfo Dataclass

30f7c07

Add CFChannel dataclass

4965b73

Use static CFProperty methods

c03b990

In favour of create_property_blah

sleep is a float

18d533c

CancelledError has no host or port properties

fd6210a

Split up commitWithThread

6c97c05

Improve logging

3cdf7df

log the whole object when available use inbuilt log formatting rather than f strings or .format (should improve performance)

jacomago force-pushed the refactor_cfstore_to_dataclasses branch from 919b9fa to 3f68066 Compare December 2, 2025 12:17

jacomago added 12 commits December 3, 2025 11:26

CFProperty use classmethods

462a7b8

CFChannel classmethod

45378c4

rename handle_old_channels to handle_channels

d0e2c4a

simpler if case

1d3204d

keyword args in using prepareFindArgs

5e5e7bc

Remove unnecessary None return

1099f01

default for channelcount

628ead3

snake case commit with thread

e6e5ffa

Add comments for overriden methods

cd33433

_commit_with_lock submethods to snake case

5d021fc

static record_info_by_name

a93ea43

reduce passing processor in static methods

e35136e

anderslindho reviewed Dec 5, 2025

View reviewed changes

anderslindho approved these changes Dec 5, 2025

View reviewed changes

jacomago added 2 commits December 5, 2025 15:49

from dict not from_cf_dict

d915481

more snake case

3b1e59d

jacomago force-pushed the refactor_cfstore_to_dataclasses branch from 211d19b to 26d6654 Compare December 5, 2025 14:49

simon-ess reviewed Dec 5, 2025

View reviewed changes

jacomago added 4 commits December 8, 2025 13:21

pvstatus to strenum

0cb5e00

cfpropertyname as strenum

62ecc7d

record property names list set

991a550

Specific error class

f74749e

jacomago force-pushed the refactor_cfstore_to_dataclasses branch from 26d6654 to f74749e Compare December 8, 2025 12:25

jacomago requested a review from anderslindho December 8, 2025 12:47

anderslindho approved these changes Dec 8, 2025

View reviewed changes

jacomago merged commit 5be70cb into master Dec 8, 2025
33 of 34 checks passed

Refactor cfstore to dataclasses #124

Refactor cfstore to dataclasses #124

Conversation

jacomago commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

simon-ess left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anderslindho left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anderslindho left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jacomago commented Dec 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynanford commented Dec 6, 2025

jacomago commented Nov 4, 2025 •

edited

Loading