ADR to introduce Builder and Strategy patterns to OpenSearch query construction#941
ADR to introduce Builder and Strategy patterns to OpenSearch query construction#941
Conversation
There was a problem hiding this comment.
I'd rate my agreement at a 4. I don't fully understand the value that the QueryStrategy interface adds (easier for developers to follow control flow, I think?), but I don't see any issues with it beyond adding an extra class.
Editing to add that, despite that minor point of confusion, I think this refactor would dramatically improve our OpenSearch code, which is long overdue!
| - Developers must follow the new abstractions to trace how a query is built. | ||
|
|
||
| - **Short-term refactor cost** | ||
| - Code must be carefully moved to avoid breaking existing behavior and VCR tests. |
There was a problem hiding this comment.
Or, possibly, move to stubs/mocks...
There was a problem hiding this comment.
Good point, I think I'd like this to work as-is with no test changes to prove it works. If we then move to mocks/stubs I'm open to it... but keeping tests stable during a refactor would be ideal if we can swing it.
matt-bernhardt
left a comment
There was a problem hiding this comment.
If I remember the scale correctly, I think I'm at a 5. I don't have any concerns that warrant noting at this point - the document reads similarly enough to what we talked about in Slack at the beginning of the week, and I'd talked myself into being okay with that.
|
@jazairi Your question about the QueryStrategy interface is one I've been waffling on to. Maybe we update this to remove that for now (a simple conditional should do the trick for now and adding the QueryStrategy Interface later if we realize we need it). Pros: it's simpler and we're not sure if we need it or if it is just abstraction for abstraction case. Would you be a 5 if we did that? (I would be) @matt-bernhardt would removing the QueryStrategy interface change your support of this ADR? The core work is the same, we just remove some abstraction that Isra and I waffly about. |
|
@JPrevost Yes, I'd be a 5 if we removed it, but I should be clear that my 4 is not "I see a problem with this" so much as "I'd like to see the implementation to find out if it makes sense." That said, it does seem easy enough to tack on later if it feels useful. |
https://mitlibraries.atlassian.net/browse/USE-372
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO