Heromodels - clarify encode/decode strategy #162

New Issue

2025-10-01T15:56:41Z

scottyeager commented

2025-10-01 15:56:41 +00:00

(Migrated from github.com)

Following up on the changes in 90d72e91c5.

Summary:

We now decode RPC params into an Args struct which is used to instantiate the final data model struct before hitting the DB
Since the Args form is different, clients must be updated (especially on the use of int epochs versus string date/times)
The set and get operations are imbalanced now: we must provide set in Args format but we get back the base data model format

Example:

curl 'http://localhost:8080/api/heromodels' \
--data-raw '{"jsonrpc":"2.0","method":"calendar_event.set","params":{"title": "Team Meeting", "start_time": "2025-01-01 10:00:00", "end_time": "2025-01-01 10:00:00", "attendees": [], "docs": [], "calendar_id": 1, "status": "published", "is_all_day": false, "reminder_mins": [15], "color": "#0000FF", "timezone": "UTC", "locations": []},"id":1}'

# Response
{"jsonrpc":"2.0","id":1,"result":1}                                           

curl 'http://localhost:8080/api/heromodels' \
--data-raw '{"jsonrpc":"2.0","method":"calendar_event.get","params":1,"id":2}'

# Response
{"jsonrpc":"2.0","id":2,"result":{
	"id":	1,
	"name":	"",
	"description":	"",
	"created_at":	1759332390,
	"updated_at":	1759332390,
	"securitypolicy":	0,
	"tags":	0,
	"messages":	[],
	"title":	"Team Meeting",
	"start_time":	1735725600,
	"end_time":	1735725600,
	"registration_desks":	[],
	"attendees":	[],
	"docs":	[],
	"calendar_id":	1,
	"status":	"published",
	"is_all_day":	false,
	"reminder_mins":	[15],
	"color":	"#0000FF",
	"timezone":	"UTC",
	"priority":	"low",
	"public":	false,
	"locations":	[],
	"is_template":	false
}}

Notice how we must provide start_time as a string, but we get back an int epoch.

So I think we need to clarify the intended strategy here:

Should RPC methods use the base data models or Args?
Do we want to be sending string based date/times over the wire?
How to make the set/get methods consistent?

Following up on the changes in https://github.com/Incubaid/herolib/commit/90d72e91c50831be3411345a4d52c77de7681279. Summary: 1. We now decode RPC params into an `Args` struct which is used to instantiate the final data model struct before hitting the DB 2. Since the `Args` form is different, clients must be updated (especially on the use of `int` epochs versus `string` date/times) 3. The set and get operations are imbalanced now: we must provide set in `Args` format but we get back the base data model format Example: ``` curl 'http://localhost:8080/api/heromodels' \ --data-raw '{"jsonrpc":"2.0","method":"calendar_event.set","params":{"title": "Team Meeting", "start_time": "2025-01-01 10:00:00", "end_time": "2025-01-01 10:00:00", "attendees": [], "docs": [], "calendar_id": 1, "status": "published", "is_all_day": false, "reminder_mins": [15], "color": "#0000FF", "timezone": "UTC", "locations": []},"id":1}' # Response {"jsonrpc":"2.0","id":1,"result":1} curl 'http://localhost:8080/api/heromodels' \ --data-raw '{"jsonrpc":"2.0","method":"calendar_event.get","params":1,"id":2}' # Response {"jsonrpc":"2.0","id":2,"result":{ "id": 1, "name": "", "description": "", "created_at": 1759332390, "updated_at": 1759332390, "securitypolicy": 0, "tags": 0, "messages": [], "title": "Team Meeting", "start_time": 1735725600, "end_time": 1735725600, "registration_desks": [], "attendees": [], "docs": [], "calendar_id": 1, "status": "published", "is_all_day": false, "reminder_mins": [15], "color": "#0000FF", "timezone": "UTC", "priority": "low", "public": false, "locations": [], "is_template": false }} ``` Notice how we must provide `start_time` as a string, but we get back an `int` epoch. So I think we need to clarify the intended strategy here: * Should RPC methods use the base data models or `Args`? * Do we want to be sending string based date/times over the wire? * How to make the set/get methods consistent?

Mahmoud-Emad commented

2025-10-01 16:59:08 +00:00

(Migrated from github.com)

The code has been reverted. I ran into issues with the way we were encoding/decoding the request and response. I attempted a workaround that also fixes the delete issue mentioned in issue #157, but it seems the code is still not working. I need to think about a better solution, so please track the mentioned issue for clearer updates on the progress.

The code has been reverted. I ran into issues with the way we were encoding/decoding the request and response. I attempted a workaround that also fixes the delete issue mentioned in [issue #157](https://github.com/Incubaid/herolib/issues/157), but it seems the code is still not working. I need to think about a better solution, so please track the mentioned issue for clearer updates on the progress.

scottyeager commented

2025-10-07 04:19:04 +00:00

(Migrated from github.com)

Here's an update on the preferred approach:

Imbalance between set/get methods is intentional
- set methods provide convenience for the caller via the Args struct and new method
- get methods return the object representation used by the db and the user might need to do extra work to fill in the details
- Example: we set tags as a list of strings, but we get tags as an id. We have to look up the tags string based on the id, if it's needed
We should always use epoch time/dates. Clients can present differently

Here's an update on the preferred approach: * Imbalance between set/get methods is intentional * set methods provide convenience for the caller via the `Args` struct and `new` method * get methods return the object representation used by the db and the user might need to do extra work to fill in the details * Example: we set tags as a list of strings, but we get tags as an id. We have to look up the tags string based on the id, if it's needed * We should always use epoch time/dates. Clients can present differently

👍 1

Mahmoud-Emad commented

2025-10-15 11:43:40 +00:00

(Migrated from github.com)

@scottyeager, please verify the implementation

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: MahmoudEmad/herolib#162