Skip to content

🐛 fix: ensure events only fire when public methods return valid data #181

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DerAndereAndi wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

🐛 fix: ensure events only fire when public methods return valid data #181

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
85dabda
feat: add comprehensive validation system for EEBUS data types
DerAndereAndi Jul 12, 2025
d7d68c5
refactor: implement unified validation framework for EEBUS measurements
DerAndereAndi Jul 12, 2025
fcf31f7
♻️ refactor: standardize measurement validation behavior and fix ptr …
DerAndereAndi Jul 12, 2025
b8e8207
✨ feat(eg/lpc): add data validation for EG LPC use case
DerAndereAndi Jul 12, 2025
eced9bb
🐛 fix: ensure events only fire when public methods return valid data
DerAndereAndi Jul 12, 2025
2882c7b
🧹 refactor: rename parameters to avoid shadowing built-in min/max fun…
DerAndereAndi Jul 12, 2025
8abf019
✅ test: improve test coverage across multiple use case packages
DerAndereAndi Jul 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion features/internal/deviceconfiguration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ func (d *DeviceConfigurationCommon) CheckEventPayloadDataForFilter(payloadData a

for _, item := range data.DeviceConfigurationKeyValueData {
if item.KeyId != nil &&
*item.KeyId == *desc.KeyId ||
*item.KeyId == *desc.KeyId &&
item.Value != nil {
return true
}
Expand Down
21 changes: 15 additions & 6 deletions usecases/eg/lpc/events.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,8 +133,11 @@ func (e *LPC) loadControlLimitDataUpdate(payload spineapi.EventPayload) {
LimitDirection: util.Ptr(model.EnergyDirectionTypeConsume),
ScopeType: util.Ptr(model.ScopeTypeTypeActivePowerLimit),
}
if lc.CheckEventPayloadDataForFilter(payload.Data, filter) && e.EventCB != nil {
e.EventCB(payload.Ski, payload.Device, payload.Entity, DataUpdateLimit)
if lc.CheckEventPayloadDataForFilter(payload.Data, filter) {
// Only fire event if public method succeeds (data is valid and retrievable)
if _, err := e.ConsumptionLimit(payload.Entity); err == nil && e.EventCB != nil {
e.EventCB(payload.Ski, payload.Device, payload.Entity, DataUpdateLimit)
}
}
}
}
Expand All @@ -155,12 +158,18 @@ func (e *LPC) configurationDataUpdate(payload spineapi.EventPayload) {
filter := model.DeviceConfigurationKeyValueDescriptionDataType{
KeyName: util.Ptr(model.DeviceConfigurationKeyNameTypeFailsafeConsumptionActivePowerLimit),
}
if dc.CheckEventPayloadDataForFilter(payload.Data, filter) && e.EventCB != nil {
e.EventCB(payload.Ski, payload.Device, payload.Entity, DataUpdateFailsafeConsumptionActivePowerLimit)
if dc.CheckEventPayloadDataForFilter(payload.Data, filter) {
// Only fire event if public method succeeds (data is valid and retrievable)
if _, err := e.FailsafeConsumptionActivePowerLimit(payload.Entity); err == nil && e.EventCB != nil {
e.EventCB(payload.Ski, payload.Device, payload.Entity, DataUpdateFailsafeConsumptionActivePowerLimit)
}
}
filter.KeyName = util.Ptr(model.DeviceConfigurationKeyNameTypeFailsafeDurationMinimum)
if dc.CheckEventPayloadDataForFilter(payload.Data, filter) && e.EventCB != nil {
e.EventCB(payload.Ski, payload.Device, payload.Entity, DataUpdateFailsafeDurationMinimum)
if dc.CheckEventPayloadDataForFilter(payload.Data, filter) {
// Only fire event if public method succeeds (data is valid and retrievable)
if _, err := e.FailsafeDurationMinimum(payload.Entity); err == nil && e.EventCB != nil {
e.EventCB(payload.Ski, payload.Device, payload.Entity, DataUpdateFailsafeDurationMinimum)
}
}
}
}
37 changes: 29 additions & 8 deletions usecases/eg/lpc/events_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package lpc

import (
"time"

spineapi "github.com/enbility/spine-go/api"
"github.com/enbility/spine-go/model"
"github.com/enbility/spine-go/util"
Expand Down Expand Up @@ -124,14 +126,23 @@ func (s *EgLPCSuite) Test_loadControlLimitDataUpdate() {
data = &model.LoadControlLimitListDataType{
LoadControlLimitData: []model.LoadControlLimitDataType{
{
LimitId: util.Ptr(model.LoadControlLimitIdType(0)),
Value: model.NewScaledNumberType(16),
LimitId: util.Ptr(model.LoadControlLimitIdType(0)),
IsLimitChangeable: util.Ptr(true),
IsLimitActive: util.Ptr(false),
Value: model.NewScaledNumberType(6000),
TimePeriod: &model.TimePeriodType{
EndTime: model.NewAbsoluteOrRelativeTimeType("PT2H"),
},
},
},
}

payload.Data = data

// Update the feature with the data so it's actually stored
_, fErr = rFeature.UpdateData(true, model.FunctionTypeLoadControlLimitListData, data, nil, nil)
assert.Nil(s.T(), fErr)

s.sut.loadControlLimitDataUpdate(payload)
assert.True(s.T(), s.eventCalled)
}
Expand All @@ -148,12 +159,14 @@ func (s *EgLPCSuite) Test_configurationDataUpdate() {
descData := &model.DeviceConfigurationKeyValueDescriptionListDataType{
DeviceConfigurationKeyValueDescriptionData: []model.DeviceConfigurationKeyValueDescriptionDataType{
{
KeyId: util.Ptr(model.DeviceConfigurationKeyIdType(1)),
KeyName: util.Ptr(model.DeviceConfigurationKeyNameTypeFailsafeConsumptionActivePowerLimit),
KeyId: util.Ptr(model.DeviceConfigurationKeyIdType(1)),
KeyName: util.Ptr(model.DeviceConfigurationKeyNameTypeFailsafeConsumptionActivePowerLimit),
ValueType: util.Ptr(model.DeviceConfigurationKeyValueTypeTypeScaledNumber),
},
{
KeyId: util.Ptr(model.DeviceConfigurationKeyIdType(2)),
KeyName: util.Ptr(model.DeviceConfigurationKeyNameTypeFailsafeDurationMinimum),
KeyId: util.Ptr(model.DeviceConfigurationKeyIdType(2)),
KeyName: util.Ptr(model.DeviceConfigurationKeyNameTypeFailsafeDurationMinimum),
ValueType: util.Ptr(model.DeviceConfigurationKeyValueTypeTypeDuration),
},
},
}
Expand All @@ -178,17 +191,25 @@ func (s *EgLPCSuite) Test_configurationDataUpdate() {
DeviceConfigurationKeyValueData: []model.DeviceConfigurationKeyValueDataType{
{
KeyId: util.Ptr(model.DeviceConfigurationKeyIdType(1)),
Value: &model.DeviceConfigurationKeyValueValueType{},
Value: &model.DeviceConfigurationKeyValueValueType{
ScaledNumber: model.NewScaledNumberType(6000),
},
},
{
KeyId: util.Ptr(model.DeviceConfigurationKeyIdType(2)),
Value: &model.DeviceConfigurationKeyValueValueType{},
Value: &model.DeviceConfigurationKeyValueValueType{
Duration: model.NewDurationType(time.Hour * 10),
},
},
},
}

payload.Data = data

// Update the feature with the data so it's actually stored
_, fErr = rFeature.UpdateData(true, model.FunctionTypeDeviceConfigurationKeyValueListData, data, nil, nil)
assert.Nil(s.T(), fErr)

s.sut.configurationDataUpdate(payload)
assert.True(s.T(), s.eventCalled)
}
20 changes: 20 additions & 0 deletions usecases/eg/lpc/public.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@
return
}

// Validate LoadControlLimitData using EG LPC validator
// This ensures data integrity and compliance with EG LPC specification requirements
if err := EGLPCLoadControlLimitValidator.Validate(value); err != nil {
resultErr = api.ErrDataInvalid
return
}

Comment on lines +63 to +69
Copy link
Contributor

@sthelen-enqs sthelen-enqs Dec 12, 2025

Choose a reason for hiding this comment

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

This does nothing that isn't already done by lines 58-61. It's not technically wrong, it's just dead code that's burning cycles.

Copy link
Contributor

@sthelen-enqs sthelen-enqs Dec 12, 2025

Choose a reason for hiding this comment

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

Is the intention here to replace the current piecemeal validators with generic validators? In that case this should replace L59-61 and then I'd need to double-check the correctness again.

limit.Value = value.Value.GetValue()
limit.IsChangeable = (value.IsLimitChangeable != nil && *value.IsLimitChangeable)
limit.IsActive = (value.IsLimitActive != nil && *value.IsLimitActive)
Expand Down Expand Up @@ -123,6 +130,12 @@
return 0, api.ErrDataNotAvailable
}

// Validate DeviceConfigurationKeyValueData using EG LPC validator
// This ensures failsafe power limit values are within reasonable ranges and properly formatted
if err := EGLPCDeviceConfigurationValidator.Validate(data); err != nil {
return 0, api.ErrDataInvalid
}

Comment on lines +133 to +138
Copy link
Contributor

@sthelen-enqs sthelen-enqs Dec 12, 2025

Choose a reason for hiding this comment

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

Same as above, except that in addition the comment is very misleading. This code doesn't actually check that power limit values are within reasonable limits. It only checks if KeyId is set (which must be set anyway if we've reached this function cal), and if a value is present that only ScaledNumber and/or Duration are set on it (but not String/Boolean/DateTime).

I'm not sure of the utility of this, and from a validation standpoint it's not even technically correct since:
a) data.Value must be set at this point (the code currently passes when it isn't),
b) I don't think the SPINE/EEBUS spec forbid having additional members of the DeviceConfigurationKeyValueValueType set at this point (such as DateTime, Boolean or String), and
c) if the spec did forbit it, the check would also need to be that ScaledNumber XOR Duration should be set on it since a failsafeConsumptionActivePowerLimit should not include a duration, and a failsafeDurationMinimum should not contain a scaledNumber

return data.Value.ScaledNumber.GetValue(), nil
}

Expand Down Expand Up @@ -188,6 +201,7 @@
return 0, api.ErrDataNotAvailable
}


Check failure on line 204 in usecases/eg/lpc/public.go

View workflow job for this annotation

GitHub Actions / Build 

File is not properly formatted (gofmt)
return data.Value.Duration.GetTimeDuration()
}

Expand Down Expand Up @@ -289,6 +303,12 @@
return 0, api.ErrDataNotAvailable
}

// Validate ElectricalConnectionCharacteristicData using EG LPC validator
// This ensures nominal consumption values are positive and within reasonable ranges
Copy link
Contributor

@sthelen-enqs sthelen-enqs Dec 12, 2025

Choose a reason for hiding this comment

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

Does it? As far as I can tell, it only validates that a characteristicId is set, and that the characteristicType is reasonable (which it has to be anyway since the filter uses the characterisicType to find the value in the first place.
Realistically this is only checking that CharacteristicId != nil (assuming our filter is functioning correctly).
Neither value nor unit are checked, therefore it's a bit of a stretch to say this ensures values are positive and within reasonable ranges.

if err := EGLPCElectricalConnectionCharacteristicValidator.Validate(&data[0]); err != nil {
return 0, api.ErrDataInvalid
}

return data[0].Value.GetValue(), nil
}

Expand Down
Loading