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

DerAndereAndi · 2025-07-12T20:14:24Z

Summary

Implement robust data validation for events to ensure consistency between event notifications and public API data availability
Fix critical logic bug in DeviceConfiguration event filtering
Ensure events only fire when corresponding public methods will return valid data

Changes Made

Core Logic Fix

Fixed DeviceConfiguration OR/AND logic bug: Corrected operator precedence in CheckEventPayloadDataForFilter that was causing false positives
Changed *item.KeyId == *desc.KeyId || to *item.KeyId == *desc.KeyId && for proper field validation

Event Handler Validation

Updated 18 event handlers across three use cases to validate data with public methods before firing events:
- EG LPC: 3 event handlers (ConsumptionLimit, FailsafeConsumptionActivePowerLimit, FailsafeDurationMinimum)
- MA MPC: 7 measurement event handlers (Power, PowerPerPhase, EnergyConsumed, EnergyProduced, CurrentPerPhase, VoltagePerPhase, Frequency)
- MA MGCP: 7 event handlers (PowerLimitationFactor + 6 measurement types)

Test Coverage Improvements

Enhanced event tests to provide complete, valid data that passes both event filters and public method validation
Added proper electrical connection setup for measurement validation
Ensured tests cover both success and failure scenarios
Added missing required fields (ValueType, ValueSource, MeasurementType, CommodityType, Duration values)

Problem Solved

Previously, events could fire for data that would then fail when accessed through public methods due to validation failures. This created
inconsistency where users would receive event notifications but then get errors when trying to retrieve the announced data.

Solution

Events now perform the same validation as public methods before firing, ensuring that:

Events only fire when data is actually retrievable via public API
No more misleading event notifications for invalid/incomplete data
Consistent behavior between event system and public method access

Test Coverage

All existing tests pass
Enhanced negative test coverage for scenarios that should not trigger events
Comprehensive validation of both event filtering and public method validation logic

Breaking Changes

None - this is a bug fix that improves reliability without changing public APIs.

Implement type-safe validation framework using Go generics: - Add generic validation system in validation.go with BaseValidator - Support for any SPINE data type with compile-time type safety - Comprehensive validation rules: RequireField, ValidateRange, ValidateEnum, etc. - Measurement-specific validation with predefined validators - Replace MeasurementWithMandatoryData with new validation approach - Complete GoDoc documentation with usage examples - Comprehensive test coverage for all validation scenarios - Markdown documentation guide for developers Benefits: - No reflection, direct field access for performance - Type-safe validators prevent runtime errors - Composable rules for complex validation scenarios - Clear error messages with validator context - Easy extension for new SPINE data types

- Create generic validation framework with Validator[T] interface - Implement BaseValidator with composable validation rules - Add MeasurementValidator for type-safe measurement validation - Unify validation through MeasurementPhaseSpecificDataForFilter - Remove non-spec frequency range validation (45-65Hz) - Add comprehensive test coverage for all validators - Achieve 100% test coverage for MPC use case The new validation system ensures spec compliance by: - Rejecting averageValue types when value is required - Rejecting empirical sources for energy measurements - Allowing any ValueState for current measurements - Validating voltage range (0-1000V) per spec - Removing frequency range check not in MPC spec This unified approach reduces code duplication and makes it easier to maintain consistent validation across all EEBUS use cases.

…function duplication - Replace local ptr[T any] functions with util.Ptr from spine-go utils package - Standardize "no valid data found" behavior to return errors consistently for both MPC and MGCP - Update MGCP test expectations to match consistent error handling - Remove duplicate utility functions across validator test files - Ensure both use cases handle data availability scenarios uniformly

Add validators for EG LPC (Energy Guard Load Power Consumption) use case to validate data structures when reading from Controllable Systems: - LoadControlLimitData: validate required fields (LimitId, Value) - DeviceConfigurationKeyValueData: validate required fields and value types - ElectricalConnectionCharacteristicData: validate required fields and characteristic types Validation follows EEBus LPC specification principles: - Only validate structural integrity and type requirements - Trust that CS has validated its own data according to spec - No arbitrary range validations beyond spec requirements This ensures data integrity when EG reads from CS devices while following the unified validation framework pattern established in MGCP and MPC use cases.

- Fix DeviceConfiguration OR/AND logic bug in CheckEventPayloadDataForFilter - Update all event handlers to validate data with corresponding public methods before firing events - Add public method validation to 18 event handlers across EG LPC, MA MPC, and MA MGCP use cases - Update event tests to provide complete valid data that passes both filters and validation - Ensure events are only triggered when data is actually retrievable via public API This prevents event notifications for data that would fail when accessed through public methods, ensuring consistency between event notifications and data availability.

…ctions - Rename 'min' and 'max' parameters to 'minVal' and 'maxVal' in validation functions - Fix CodeFactor warnings about redefinition of built-in functions - No functional changes, improves code maintainability

DerAndereAndi · 2025-07-12T20:32:31Z

@sthelen-enqs this should also help for the scenario in your PR enbility/spine-go#52

- Increased internal/measurement.go coverage from 69% to 94.1% - Enhanced eg/lpc package coverage from 94.7% to 95.2% - Boosted ma/mgcp package coverage from 94.7% to 97.3% - Achieved 100% coverage for ma/mpc package (up from 97.4%) Added comprehensive test cases for: - Measurement validation functions and edge cases - LPC validator functions and configuration handling - MGCP/MPC event callbacks for phase-specific measurements - Error conditions and invalid data scenarios

sthelen-enqs

Hi,

I haven't fully checked through the whole PR, but found a couple of minor nits in the tests that might be nice to fix. I'll check over the rest of the validation later.

general nit: could you run gofmt on the test cases as well?

sthelen-enqs · 2025-12-12T10:35:10Z

usecases/eg/lpc/public_test.go

+	assert.Nil(s.T(), fErr)
+
+	// Test 1: Missing LimitId - should return ErrDataInvalid when validator runs
+	// Note: Data must include Value to pass the `value.Value == nil` check in public.go:59


This comment is incorrect, and confusing. GetLimitDataForId will return nil, api.ErrDataNotAvailable because the filter won't match any limits and len(result) will be 0. The check on L59 then short-circuits on err != nil and value.Value will never be checked.

Suggested change

// Note: Data must include Value to pass the `value.Value == nil` check in public.go:59

sthelen-enqs · 2025-12-12T10:35:24Z

usecases/eg/lpc/public_test.go

+				// LimitId missing - this should trigger validation error
+				IsLimitChangeable: util.Ptr(true),
+				IsLimitActive:     util.Ptr(false),
+				Value:             model.NewScaledNumberType(6000), // Include value to pass initial check


Suggested change

Value: model.NewScaledNumberType(6000), // Include value to pass initial check

Value: model.NewScaledNumberType(6000),

sthelen-enqs · 2025-12-12T11:11:02Z

usecases/eg/lpc/public_test.go

+	assert.Equal(s.T(), time.Duration(time.Hour*2), data)
+}
+
+func (s *EgLPCSuite) Test_ConsumptionNominalMax_ValidationErrors() {


This test case is using the wrong CharacteristicType throughout because the "current" version uses the DeviceType to determine the correct CharacterisicType, but the DeviceType is never initialised in the test suite (and therefore nil).

Could you maybe rebase this PR on fix/use-correct-eg-lpc-lpp-characteristic-types ?

sthelen-enqs

I have to admit I'm not in love with these changes. The validator logic feels more complicated than necessary, and I fear will make it easier to filter data incorrectly as evidenced by the nits and spec inconsistencies I've found so far.
I, however, do not have a better idea as of right now either.

sthelen-enqs · 2025-12-12T12:08:58Z

usecases/eg/lpc/public.go

 	}

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


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.

sthelen-enqs · 2025-12-12T12:12:13Z

usecases/eg/lpc/public.go

+	// 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
+	}
+


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.

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.

sthelen-enqs · 2025-12-12T12:32:27Z

usecases/eg/lpc/public.go

+	// 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
+	}
+


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

sthelen-enqs · 2025-12-12T12:41:45Z

usecases/internal/measurement.go

+	// Validate validator parameter
+	if validator == nil {
+		return nil, fmt.Errorf("validator is required")
+	}
+


Could we make validator pass-by-value instead of pointer typed? Then it cannot be nil by type guarantee which reduces the risk of errors at runtime and (imo) makes it easier to reason about.

I don't think the struct is large enough that we have to worry about passing it by value, and I think the go compiler should just do the right thing™

Not a blocker in any way, just wondering if we can improve the ergonomics here.

sthelen-enqs · 2025-12-12T12:52:51Z

usecases/internal/measurement.go

+			// For MGCP-003 compliance, we explicitly handle ErrSkipMeasurement
+			// All other validation errors also result in skipping the measurement
+			continue // Skip invalid measurements, don't fail entire operation


Although the reasoning is correct, this error message is confusing as no error is handled explicitly.
It might be nice to have some way of logging/warning/reporting measurements with valueState != normal and probably another one for other validationErrors, but I don't know if we need that in this PR.

Suggested change

// For MGCP-003 compliance, we explicitly handle ErrSkipMeasurement

// All other validation errors also result in skipping the measurement

continue // Skip invalid measurements, don't fail entire operation

// Due to MGCP-003, and MPC-003 we want to skip measurements with valueState != normal

// We also want to skip all other validation errors so that we don't fail the entire operation

// just because the remote contains invalid data

continue

sthelen-enqs · 2025-12-12T14:19:33Z

usecases/ma/mpc/validators.go

+	WithRule(internal.RequireMeasurementId()).
+	WithRule(internal.RequireMeasurementValue()).
+	WithRule(internal.RequireValueType(model.MeasurementValueTypeTypeValue)).
+	WithRule(internal.RequireValueSource(currentSourceTypes...)).


Afaict valueSource is always required for the MU and can be made mandatory here

sthelen-enqs · 2025-12-12T14:19:45Z

usecases/ma/mpc/validators.go

+	WithName("MPC Voltage").
+	WithRule(internal.RequireMeasurementId()).
+	WithRule(internal.RequireMeasurementValue()).
+	WithRule(internal.RequireValueType(model.MeasurementValueTypeTypeValue)).


Afaict valueSource is always required for the MU and can be made mandatory here

sthelen-enqs · 2025-12-12T14:19:56Z

usecases/ma/mpc/validators.go

+	WithRule(internal.RequireMeasurementId()).
+	WithRule(internal.RequireMeasurementValue()).
+	WithRule(internal.RequireValueType(model.MeasurementValueTypeTypeValue)).
+	WithRule(internal.RequireValueSource(frequencySourceTypes...)).


Afaict valueSource is always required for the MU and can be made mandatory here

sthelen-enqs · 2025-12-12T14:20:15Z

usecases/ma/mpc/validators.go

+
+// getMeasurementValue is a helper that validates and extracts the value from measurements
+// DEPRECATED: Use MeasurementPhaseSpecificDataForFilter with validators instead
+func getMeasurementValue(measurements []model.MeasurementDataType, validator *internal.MeasurementValidator) (float64, error) {
+	return internal.GetMeasurementValue(measurements, validator)
+}


If this is deprecated, can we just get rid of it?

sthelen-enqs · 2025-12-12T14:21:04Z

usecases/ma/mpc/validators.go

+	WithRule(internal.RequireMeasurementId()).
+	WithRule(internal.RequireMeasurementValue()).
+	WithRule(internal.RequireValueType(model.MeasurementValueTypeTypeValue)).
+	WithRule(internal.RequireValueSource(voltageSourceTypes...)).


Because of MPC-003 shouldn't this be an equivalent SkipValueState as in mgcp/validators?

DerAndereAndi added 6 commits July 12, 2025 13:28

sthelen-enqs self-requested a review October 22, 2025 12:54

sthelen-enqs requested changes Dec 12, 2025

View reviewed changes

	Value: model.NewScaledNumberType(6000), // Include value to pass initial check
	Value: model.NewScaledNumberType(6000),

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

DerAndereAndi commented Jul 12, 2025

Uh oh!

DerAndereAndi commented Jul 12, 2025

Uh oh!

sthelen-enqs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sthelen-enqs Dec 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sthelen-enqs left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sthelen-enqs left a comment •

edited

Loading

sthelen-enqs Dec 12, 2025 •

edited

Loading

sthelen-enqs left a comment •

edited

Loading