Skip to content

OCPP: serialize dateTime with fractional seconds#31191

Open
premultiply wants to merge 3 commits into
masterfrom
ocpp-datetime-milliseconds
Open

OCPP: serialize dateTime with fractional seconds#31191
premultiply wants to merge 3 commits into
masterfrom
ocpp-datetime-milliseconds

Conversation

@premultiply

@premultiply premultiply commented Jun 24, 2026

Copy link
Copy Markdown
Member

Changes the OCPP dateTime serialisation format to RFC3339 with fractional seconds (2026-06-23T12:45:09.404Z).

Previously, ocpp-go used the default time.RFC3339 (second granularity, no fractional part).

Background: Some chargers (e.g. Webasto/Ampure NEXT) reject timestamps without a fractional part.

Fixes part of #31158

Copilot AI review requested due to automatic review settings June 24, 2026 20:55

This comment was marked as low quality.

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've left some high level feedback:

  • Overriding types.DateTimeFormat in an init() function introduces a hidden global side effect; consider either exposing an explicit initializer or wiring this through configuration so consumers of the types package are aware that the format is being changed globally.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Overriding `types.DateTimeFormat` in an `init()` function introduces a hidden global side effect; consider either exposing an explicit initializer or wiring this through configuration so consumers of the `types` package are aware that the format is being changed globally.

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@premultiply premultiply marked this pull request as draft June 24, 2026 20:59
@premultiply premultiply changed the title OCPP: serialize dateTime with fractional seconds for all chargers OCPP: serialize dateTime with fractional seconds Jun 24, 2026
@premultiply premultiply self-assigned this Jun 24, 2026
@premultiply premultiply added the devices Specific device support label Jun 24, 2026
@premultiply premultiply marked this pull request as ready for review June 24, 2026 21:05

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey - I've found 1 issue, and left some high level feedback:

  • Overriding the global types.DateTimeFormat in init can be surprising for other OCPP usages in the same process; consider either scoping this behavior behind a configuration flag or at least documenting this side effect clearly where Config or the package is consumed.
  • Instead of a custom dateTimeFormat string, consider using time.RFC3339Nano and then constraining precision (e.g., truncating to milliseconds) at the time.Time level to rely on the standard library’s formatting guarantees and avoid drift if RFC3339 handling evolves.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- Overriding the global `types.DateTimeFormat` in `init` can be surprising for other OCPP usages in the same process; consider either scoping this behavior behind a configuration flag or at least documenting this side effect clearly where `Config` or the package is consumed.
- Instead of a custom `dateTimeFormat` string, consider using `time.RFC3339Nano` and then constraining precision (e.g., truncating to milliseconds) at the `time.Time` level to rely on the standard library’s formatting guarantees and avoid drift if RFC3339 handling evolves.

## Individual Comments

### Comment 1
<location path="charger/ocpp/instance_test.go" line_range="41-43" />
<code_context>
+	require.Equal(t, dateTimeFormat, types.DateTimeFormat)
+
+	ts := types.NewDateTime(time.Date(2026, 6, 23, 12, 45, 9, 404_000_000, time.UTC))
+	b, err := json.Marshal(ts)
+	require.NoError(t, err)
+	require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b))
+}
</code_context>
<issue_to_address>
**suggestion (testing):** Add a round-trip (marshal + unmarshal) test to ensure deserialization remains compatible

Right now we only assert the marshalled JSON string. Please also unmarshal that JSON back into `types.DateTime` (or the relevant type) and assert it matches the original time (accounting for location if needed) so we verify decode as well as encode with fractional seconds.

Suggested implementation:

```golang
func TestMain(m *testing.M) {
		}
	}
}

func TestDateTimeFormatHasFractionalSeconds(t *testing.T) {
	require.Equal(t, dateTimeFormat, types.DateTimeFormat)

	original := types.NewDateTime(time.Date(2026, 6, 23, 12, 45, 9, 404_000_000, time.UTC))

	// Marshal to JSON and verify the wire format (including fractional seconds)
	b, err := json.Marshal(original)
	require.NoError(t, err)
	require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b))

	// Unmarshal back and ensure we get the same instant in time
	var roundTripped types.DateTime
	err = json.Unmarshal(b, &roundTripped)
	require.NoError(t, err)

	// Compare underlying time values to verify round-trip correctness
	require.True(t, time.Time(original).Equal(time.Time(roundTripped)))
}

```

The assertion `time.Time(original).Equal(time.Time(roundTripped))` assumes that `types.DateTime` is defined as `type DateTime time.Time`. If the actual definition differs (for example, if it is a struct wrapper or exposes a method to retrieve the underlying `time.Time`), adjust the comparison accordingly, e.g. `original.Time().Equal(roundTripped.Time())` or another equivalent accessor.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines +41 to +43
b, err := json.Marshal(ts)
require.NoError(t, err)
require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

suggestion (testing): Add a round-trip (marshal + unmarshal) test to ensure deserialization remains compatible

Right now we only assert the marshalled JSON string. Please also unmarshal that JSON back into types.DateTime (or the relevant type) and assert it matches the original time (accounting for location if needed) so we verify decode as well as encode with fractional seconds.

Suggested implementation:

func TestMain(m *testing.M) {
		}
	}
}

func TestDateTimeFormatHasFractionalSeconds(t *testing.T) {
	require.Equal(t, dateTimeFormat, types.DateTimeFormat)

	original := types.NewDateTime(time.Date(2026, 6, 23, 12, 45, 9, 404_000_000, time.UTC))

	// Marshal to JSON and verify the wire format (including fractional seconds)
	b, err := json.Marshal(original)
	require.NoError(t, err)
	require.JSONEq(t, `"2026-06-23T12:45:09.404Z"`, string(b))

	// Unmarshal back and ensure we get the same instant in time
	var roundTripped types.DateTime
	err = json.Unmarshal(b, &roundTripped)
	require.NoError(t, err)

	// Compare underlying time values to verify round-trip correctness
	require.True(t, time.Time(original).Equal(time.Time(roundTripped)))
}

The assertion time.Time(original).Equal(time.Time(roundTripped)) assumes that types.DateTime is defined as type DateTime time.Time. If the actual definition differs (for example, if it is a struct wrapper or exposes a method to retrieve the underlying time.Time), adjust the comparison accordingly, e.g. original.Time().Equal(roundTripped.Time()) or another equivalent accessor.

@andig

andig commented Jun 25, 2026

Copy link
Copy Markdown
Member

Afaiu OCPP uses ISO8106 (or seomthing like this) which is much more lose. Implication: RFC3339 without fractional seconds is perfectly valid. We can try this but I‘m convinced that it will just break the next charger in no time and we’ll have to revert it. What the library doesn‘t allow is format relaxations per charger. In any case, users should open issues with charger vendors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devices Specific device support

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants