Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE] Move ExtensionRestResponse class to same package as ExtensionRestRequest #165

Closed
dbwiddis opened this issue Sep 30, 2022 · 2 comments · Fixed by #169 or #187
Closed

[FEATURE] Move ExtensionRestResponse class to same package as ExtensionRestRequest #165

dbwiddis opened this issue Sep 30, 2022 · 2 comments · Fixed by #169 or #187
Assignees
Labels
enhancement New feature or request

Comments

@dbwiddis
Copy link
Member

Is your feature request related to a problem?

As part of #163, the ExtensionRestRequest was moved to the OpenSearch project. It makes sense to also relocate the ExtensionRestResponse object there as well for Reasons:

  • Put the request and response together in the same package
  • Enable the header keys to be made public constants and easily read by the RestSendToExtensionAction rather than duplicating constants (code smell, error prone).

What solution would you like?

Move the class and its tests to org.opensearch.extensions.rest package.

What alternatives have you considered?

Leaving it as is and hoping nobody changes one of the constants without forgetting to change the other.

Do you have any additional context?

@peternied wants to rename some methods in the class per this comment, we can handle that at the same time:

This name is too literal and kind of tripping me up, what about breaking this functionality apart into something like markParameter(s)Consumed(String([]) param), markContentConsumed(), and finally record/reportConsumedItems()

@dbwiddis dbwiddis added enhancement New feature or request good first issue Good for newcomers hacktoberfest Global event that encourages people to contribute to open-source. labels Sep 30, 2022
@dbwiddis
Copy link
Member Author

what about breaking this functionality apart

I think the confusion is probably because I am trying to leverage an (otherwise unused and thus poorly documented) superclass functionality (custom headers) for this.

Another (possibly better) option is to change this class from a subclass of BytesRestResponse and simply add the list of consumed params and the consumed content boolean as fields. Then they can be sent and retrieved via normal transport with their original names corresponding directly to the RestRequest field equivalents.

@dbwiddis dbwiddis self-assigned this Oct 3, 2022
@dbwiddis dbwiddis removed good first issue Good for newcomers hacktoberfest Global event that encourages people to contribute to open-source. labels Oct 3, 2022
@dbwiddis
Copy link
Member Author

dbwiddis commented Oct 7, 2022

Another (possibly better) option is to change this class from a subclass of BytesRestResponse and simply add the list of consumed params and the consumed content boolean as fields.

We do need a subclass of (eventually) RestResponse of which BytesRestResponse is really the only effective implementation. So extending is the only option other than repeating a ton of implementation.

Which means we can't also extend the TransportResponse because Java doesn't allow multiple inheritance.

However, while attempting to write a comment explaining why we were using the headers, I realized it was a completely self-inflicted limitation having initially implemented based on RestResponse. Once we changed the API to our own class we can just add fields to it and copy those to the corresponding TransportAction. It is a bit duplicative but the only real way around the multiple inheritance issue. And the end result is actually pretty clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
1 participant