TimeWindow Recurrence Settings shouldn't throw

[rossgrambo]

The library normally returns false when the state of a filter is invalid. The Recurrence settings instead throw when this happens. We should warn and return false instead.

[zhiyuanliang-ms]

The reason why we throw instead of warn is that the recurrence schema is quite complicated and it is easy to make mistake and it cannot immediatly be aware that the recurrence doesn't work as expected. That's why we throw to make who write the time window settings by themselves more easier to aware mistake. For those who use App Config, we have portal for them, they won't need to write recurrence settings.

We have written schema for all built-in filters: https://github.com/microsoft/FeatureManagement/tree/main/Schema/FeatureFilters
How about also validating targeting filter and throw when it is invalid?

[rossgrambo]

I'm against any sort of refresh -> leading to an exception. Still possible via the portal with advanced edit/import/etc.

If we want to offer an option to enable throwing (like IgnoreMissingFeatureFilters but IgnoreInvalidFilters), I think that's a better option to allow both preferences

[zhiyuanliang-ms]

I like providing IgnoreInvalidFilters option.

[jimmyca15]

Targeting filter throws if settings are invalid. Percentage filter doesn't, although the percentage filter is not documented anymore.

So it's hard to say what's 'normal' since there seems to be historical inconsistency on the approach. However, our TargetingFilter is certainly the most used filter.

Personally, I prefer to throw. Any configuration changes must be vetted in a pre-prod environment.

[zhenlan]

Are we consistent within the TimeWindow filter? Do we throw only for invalid recurrence settings but not for others?

[zhiyuanliang-ms]

Are we consistent within the TimeWindow filter? Do we throw only for invalid recurrence settings but not for others?

For non-recurring time window filter. We do have some invalid settings on the portal. But for this library, we actually don't have any hard constraint for time window settings before we introduced recurrence. This is the code for v2.x. We only log warning when start and end both are not specified.

So, I think we are consistent within the TimeWindow Filter.