-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Remove deprecated _source_exclude and _source_include from get API spec #42188
Conversation
cc @elastic/es-clients |
Pinging @elastic/es-core-features |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, this should have been removed in #35097
@javanna the fix should be backported also to
|
@delvedor the removal happened in 7.0, hence I don't think this should go in 6.8, but only up until 7.0. |
Removing this from the spec in These should be documented as See: #41444 |
@Mpdreamz these params are no longer supported in the codebase though, since 7.0. |
@javanna I get that but we shipped them in the spec for |
so we should leave params that we do not support in our spec? Aren't they rejected if they are provided? |
As far as I can see we deprecated them in FetchSourceContext in 6.6, and removed their support from 7.0 as part of #35097, which was indeed a breaking change. The spec should have been adapted as part of that change? |
Yes we should leave them as they have downstream breaking effects on clients if simply removed: "_source_exclude": {
"type": "list",
"description": "A list of fields to exclude from the returned _source field",
"deprecated": {
"version": "7.0.0",
"description": "Removed on the server in 7.0 and won't be accepted, erroneously documented in the spec"
},
},
"_source_include": {
"type": "list",
"description": "A list of fields to extract and return from the _source field",
"deprecated": {
"version": "7.0.0",
"description": "Removed on the server in 7.0 and won't be accepted, erroneously documented in the spec"
},
}, Ideally #35097 should have removed this before we released |
I am not following: we should have removed these parameters from the spec before 7.0 was released, but they were forgotten. Yet you are recommending to keep them around although they can't be used? I think this change should be backported to all 7.x branches. |
I am saying the spec should not introduce breaking changes in a minor. If the spec documents wrong parameters they should be deprecated and removed in a major. Similar if we ship with a typo in the java rest parser we would not remove the typo in a minor either. When the spec is published this results in generated code in other repositories. Removing parameters in the spec in minors results in breaking changes in minors for those repositories which in most cases is not acceptable. Thus either those repositories need to manually write patches to handle work around the breaking change or we deprecate in the json spec with clear messaging. I want the low level clients to be ready before ES releases rather then after so we need to be able to get rid of manual work where needed and be sure the spec does not introduce breaking changes.
If we ship mistakes we need to address these other than simply deleting the documentation for a url/part/parameter. |
For reference: I had opened #41439 to solve this through deprecations already. |
I understand and agree on the idea behind documenting deprecations in the spec, which we have been missing up until now,. But I think that what you suggest is a misuse of deprecations, because effectively code and spec would not be aligned, and the only way to align them would be to remove the unsupported param. We can't rely on the description of the parameter saying "the parameter is removed from the server code" while the param itself is documented deprecated. With this specific change, if we had the option to mark parameters deprecated earlier, we would have done that in 6.6 when we made the change, instead of just switching the old param to the new one in the spec. Marking this deprecated now only for the get api is inconsistent and I don't see where it helps. Though again I agree on using deprecations properly in the future whenever we deprecate params and such. But let's strive to have code and spec aligned at all times. |
Wholeheartedly agree, we will never get there if we treat the spec as something that can be patched up later though. E.g by removing parameters in a minor.
Again the spec results in released code e.g
now 7.1 will be released the code will be regenerated and now
This should be one of the use cases for By treating the specs as code with the same no breaking mentality will mean that mistakes (which of course will happen) are painful in the
|
If we do mark the option deprecated in the spec like you suggested, although it's already gone, and introduce this inconsistency in the spec, how would you adapt your client to such spec change? |
We would emit a compiler warning on the usage of |
To be fully pedantic the inconsistency was not introduced by adding a |
Isn't the option completely gone in 7.x on the server side though? If so, and you just generate a deprecation warning in the clients then I would still expect the features to work in the clients, but they would break. That isn't deprecation, that is completely broken which kind of defeats the purpose of marking something as deprecated. I agree with @Mpdreamz that it has got to be a lot more important for the spec to be right and to make damn sure you didn't ship mistakes. But unless Elasticsearch itself is going to re-add support for the missing parameter then I do not agree with just marking it as deprecated in the clients to fix this situation. |
but users are free to use it and it would not work as server does not support it? @ejsmith I completely agree with you. We should have done things right, but since we have not, we are not going to fix that by deprecating a removed parameter in the spec. We will surely be more careful in the future. |
I'll stop nagging after this but I think the only proper fix is to deprecate AND reintroduce the param for get only with a deprecation header. The fact it's documented and throws is a bug in 7.0, the fix is not removing it in a minor if we take the bwc guarantees of the spec serious. |
@Mpdreamz once again, we can't reintroduce a param that is not in the codebase, or the spec becomes something on its own while it should describe its corresponding code, which I believe is the goal of the spec in the first place. Also, note that these parameters were removed from other APIs too in the same way, while here the discussion focuses on get API only. It is unfortunate that we replaced the parameters in 7.0 without using deprecations in the spec (which we did for all the APIs, not only for get), and even more so that we left the old parameters in the get API spec while they should have been removed. We can't go back in time though and while we would do things differently today, we have to accept that mistakes happen and live with their consequences. Here the best thing to do for users, would be to adapt the clients in their next bugfix release and shortcut the removed parameters to their new variant. It is not perfect, but it allows to ease pain for users without having the spec diverge from the code. |
@javanna I feel we have a fundamental disconnect that is not being resolved.
You argue for pushing the short circuit on the clients, I am arguing to push that short circuit to the server. Mistakes totally happen I am not having a go at that at all. I fully realize this affects If we want to position the clients as release ready the day ES releases we need to eliminate cases exactly like this and stop treating the spec as something that can be patched up later. When a typo happens in the java parser and we document it on elastic.co we would not remove the typo in a minor either. Of course we can and will patch our way around this but I want to instill the notion that we can't just do breaking changes to the spec. The spec and code should totally not diverge so here the code should catch up with the spec since the spec already shipped. |
Your PR was not doing any short circuiting on the server though, it was rather marking unsupported params as deprecated with a textual description saying "these are actually removed from the codebase". As I said, these params were removed in 7.0 from the codebase without being deprecated first in the spec, which I think is the initial problem. I don't even know if deprecations in the spec were a thing at the time (the initial deprecation where the spec were changed was made against 6.6). As I said we would do things differently today. With this said it is not a matter of short circuiting on the server or on the client. The short circuiting can only be done on the clients at this stage because the params have been removed from the server codebase long ago, unless you are proposing to re-introduce the params to the server codebase for the whole 7.x series (then spec would need updating too obviously) and for all the APIs touched, not only get API. If that's what you mean, please open a new issue and get relevant people involved. That would be a completely different proposal compared to what we have discussed up until now in this thread. |
heya @philkra good question, depends on who we is :) In short, we should not have had an unsupported parameter in the spec in the first place, that was a leftover. I believe this could have been caught earlier during clients testing. |
@philkra, as @Mpdreamz has been saying, it's critically important that the spec stay in sync. We know people make mistakes so the only way to really make sure this doesn't happen is to get the code to be the spec. I don't know anything about how the spec is currently built and maintained, but the spec absolutely has to always be right and current if the clients are going to be able to generated from that spec and kept up to date in real-time with the server side of Elastic. |
Pinging @elastic/es-core-infra |
In #33475
_source_exclude
and_source_include
have been deprecated.In every other file in the spec only the new value is present, while in
get.json
there is also the deprecated one. This pr aligns this file to the rest of the spec.This fix should be backported in
6.x
as well.