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

Adding setters or making them public in ActionRequests #8123

Conversation

pickypg
Copy link
Member

@pickypg pickypg commented Oct 16, 2014

MoreLikeThisRequest, AnalyzeRequest, and DeleteIndexTemplateRequest (and associated builders) all prevented some fields from being set outside their constructor. This makes those fields publically settable, which helps to simplify usage in other languages like Groovy.

Closes #8122

MoreLikeThisRequest, AnalyzeRequest, and DeleteIndexTemplateRequest (and associated builders) all prevented some fields from being set outside their constructor. This makes those fields _publically_ settable, which helps to simplify usage in other languages like Groovy.
@colings86
Copy link
Contributor

Although helps to simplify things in languages like groovy I am concerned about the impact it has on the Java API. I would assume that the reason these fields are explicitly prevented from being set outside the constructor is that they are required parameters (e.g. you must specify the name of the template you want to delete). By making them optional in the Java API we increase the risk that someone will accidentally not set it causing a headache for the user. Furthermore, since this fields have always been guaranteed to be set I would be wary of unknown side-effects if the request did get made with those fields set to null. The code may not guard against these fields being null, at best this would result in a NPE, at worst the null could be interpreted as all and have serious side-effects.

@pickypg
Copy link
Member Author

pickypg commented Feb 3, 2015

@colings86 I think that they were overlooked because it was already added to the Validation API. Honestly, it should just fail at the constructor rather than being a surprise later if we don't change it.

Except MoreLikeThisRequest, which apparently does not use the Validation API and I suspect that it should.

@javanna
Copy link
Member

javanna commented Feb 3, 2015

I think all of the requests should ideally do validation using the validate method. If MoreLikeThisRequest doesn't we should fix it. That said some of them can enforce required arguments by making them required in their constructor. But I am not against adding setters too for them, as long as validate method works properly, for consistency reasons. I tend to be against adding empty constructors though to those requests, especially if using the @Nullable annotation for required fields. Does this make sense?

@s1monw
Copy link
Contributor

s1monw commented Mar 20, 2015

@javanna can you take care of this and work with @pickypg towards closing / merging, thanks!

@javanna
Copy link
Member

javanna commented Mar 21, 2015

Do you have time to address my comments @pickypg ? Let me know if you have any question.

@javanna
Copy link
Member

javanna commented May 27, 2015

I pushed a fix for this: note that the more like this api is gone in master, thus no need to modify anything there. AnalyzeRequest already had text setter after #3023 was resolved. I added the name setter to DeleteIndexTemplateRequest. Also made public default constructors for AnalyzeRequest and DeleteIndexTemplateRequest, given that through their corresponding builder it is already possible to create a request without required parameters, the validate method will throw error though.

@pickypg pickypg deleted the feature/make-actionrequests-more-consistent-8122 branch June 22, 2015 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some ActionRequests require constructor args
4 participants