search

RedHatInsights / insights-api-common-rails

Header, Encryption, RBAC, Serialization, Pagination and other common behavior for Insights microservices built with Rails
Apache License 2.0
3 stars 25 forks source link

Improve serializer to exclude attributes that should not be needed #181

Closed eclarizio closed 4 years ago

eclarizio commented 4 years ago

In Catalog, we have v1.1 and v1.0 api docs, v1.1 has a field called "metadata" that could be a potentially very expensive computation. Right now, this serializer currently still computes it and then throws it away later.

This change excludes the attribute from computation beforehand, and then does any transformation on the resulting set as before.

Also like half of the serializer file wasn't tested so I added a bunch of tests for that. In particular, lines 14 and 19 in the old file seemed to completely contradict each other and I couldn't write a test scenario for how line 19 would ever get executed, so I removed that line.

Line 14 used to read: next if attrs[name].nil?, so if something that didn't exist like "hello" got passed in, this would move on to the next attribute, causing line 19 not to be executed. If it did exist, but was nil, (something like "updated_at"), it would also move on to the next attribute. If it exists but isn't nil, then down on line 19 where it checks !attrs.key?("updated_at") && would be false and thus line 19 wouldn't get executed. There's no scenario where it doesn't exist and somehow gets to line 19, as far as I can tell.

@syncrou @lindgrenj6 Please review and/or tag others.

eclarizio commented 4 years ago

@bdunne Thanks for the feedback, updated. I'm mainly concerned about the change of removing line 19. Do you think there's any scenario where that line was executed or maybe it was simply overlooked when refactoring from something else?

bdunne commented 4 years ago

@bdunne Thanks for the feedback, updated. I'm mainly concerned about the change of removing line 19. Do you think there's any scenario where that line was executed or maybe it was simply overlooked when refactoring from something else?

Looking through the history now I'm very confident that it's not used. It was added in 3ed03b6f1fd97a6aa8a0e5c0152c870a7e086b1e for the original tagging implementation (which is no longer valid) then updated in 2b3c5d184d6475ef5b74305ce4adc9129ce36abb to not expose encrypted columns and the next if... was added in 0e022207d6a043f138baf12b7eb3f71752898306. So, I'm :+1: for removing it.