jfroelich / rss-reader

A simple Chrome extension for viewing RSS feeds
Other
10 stars 0 forks source link

Move validate and sanitize helpers back into db operations #797

Closed jfroelich closed 5 years ago

jfroelich commented 5 years ago

I am referring to functions like sanitize-entry and validate-feed. Operations like create-feed should call these functions prior to storage.

The database module layer should enforce its own constraints, and not rely on the caller to enforce its constraints.

I originally implemented it in this way, but undid it, because of what I refer to as a the conditional-execution anti-pattern. This pattern is basically a function that conditionally executes some statements based on whether a parameter is true/false. The idea is that if the caller wants to carry out those statements, they call them first, then call the function, and if they do not want to carry out those statements, they just call the function. This removes the need for the boolean parameter.

However, here, I am not so sure the boolean was entirely appropriate. The thinking is that the db module should enforce its constraints. The only reason to not do so was for the few use cases where the db module operations interacted with each other, where the data passed around in parameters was trusted (e.g. as non-user input data). For example, an operation that uses the db module to load some data, do something to it, then write it back to the database, it knows at all times the state of this data and there is no need to revalidate. Validation is expensive and always performing validation here is a waste because we "know" the data was never untrusted. It already underwent validation when it was created the first time.

So my thoughts there are that maybe that is a premature optimization. Maybe it is ok to always revalidate. Maybe the db module should not even trust itself, basically. Maybe it is better to second guess the workings of an operation that reads, changes, then writes back data, even though it is not coming from the user, on the off chance the operation itself is somehow invalidating the data.

Alternatively, maybe it is just better to violate the conditional-execution-with-boolean-parameter antipattern here. db operations like create-feed should provide a parameter like validate (boolean) that defaults to true, and if true, does validation.

Note that I already second guessed myself with the introduction of the normalization functionality. That I hardcoded into the db module ops and the caller does not get a choice to disable it. So why the different treatment? Essentially, I think the db operations should all be concerned with normalization of data, sanitization of data, and validation of data.