Skip to content

Navigation Menu

Sign in
Sign up

#220 Fix case-sensitivity of HTTP headers in models - #364

Open
parawanderer wants to merge 2 commits into
aws:events-v4-serialization-v2 from
parawanderer:fix_headers_insensitive
Open

#220 Fix case-sensitivity of HTTP headers in models #364
parawanderer wants to merge 2 commits into
aws:events-v4-serialization-v2 from
parawanderer:fix_headers_insensitive

Conversation

@parawanderer

@parawanderer parawanderer commented Aug 10, 2022

Copy link
Copy Markdown

Issue #, if available: #220

Description of changes:

This solution builds upon @jeromevdl 's solution of PR 234 to fix the
issue of headers being case sensitive. Fixes @carlzogh 's comment
regarding breaking changes in setHeaders.

This adds some extra unit tests, and reorders some of the added unit test assertions of the original PR, too

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Original PR: #298, I closed that PR by accident due to deleting and re-creating the branch from v4 as a base

richarddd commented Aug 11, 2022
edited
Loading

Copy link
Copy Markdown
Contributor

It don't see the value of adding a wrapper around the Tree Map. The only additional feature it has is mergeOrReplace which is always called statically. The factory method could be extracted (if even needed at all) from the class and TreeMap could be used direly

parawanderer commented Aug 12, 2022
edited
Loading

Copy link
Copy Markdown
Author

@richarddd This PR/code is not solely mine, but in my opinion I see the value in the original author's choosing of a dedicated class to represent HTTP Headers. Observe that that wrapped TreeMap uses a particular case setting parameter (case insensitivity) rather than just being any Map/TreeMap.

You cannot infer this detail from just the fact of using any Map/TreeMap. So configuring this in serialization would be (imo) uglier with either being configured in some sort of config builder outside the class (which makes it harder to understand how the class actually works at a glance), or using some sort of annotation-based specifier on every class member to do with http headers, of which there are a lot. (imo this deals with the "identifying at a glance" issue better but calls for a lot of repetition/low maintainability)

From my perspective as a user of the library, the wrapper allows for some useful documentation. If I need to deep-dive into the library/code, I could see that I'm always dealing with the same special type of map that particularly implements the properties of HTTP Headers.

For the maintainers of the library, maintaining/adding additional properties to this particular Map is centralised, and would probably integrate more easily into any typical serialization framework. E.g. changing the internal implementation to maybe use a regular hashmap with case-insensitivity built in in some other way is also easier.

I see the argument of there being a bunch of generic code/duplication, and could see a perspective of just extending TreeMap and providing that constructor parameter to the parent constructor. But in that case you could end up with people using the map as a TreeMap potentially expecting the properties of a TreeMap, when the fact that it is such is coincidence and could be changed at a later time. So in my opinion it's still an useful level of abstraction that helps maintainability there, too.

WDYT?

EDIT: I got completely sidetracked and forgot about using a factory. I think this is part of that "configuration being hard to follow/maintain" issue too, though. (Unless there's a way to do that to sidestep both issues, of course)

Copy link
Copy Markdown
Contributor

WDYT?

Ah right, i did not think about serializing/deserializing makes sense to have then as a wrapper class

parawanderer reacted with thumbs up emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andclt andclt Awaiting requested review from andclt
1 more reviewer
@smirnoal smirnoal smirnoal approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /