Skip to content

Add Stringified marker to Tokenizer and TokenFilter properties #2222

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

Merged
merged 1 commit into from
Aug 15, 2023

Conversation

flobernd
Copy link
Member

@flobernd flobernd commented Aug 3, 2023

These properties seem to be sent as string sometimes. Judging from the server code, it even seems like more (all?) properties of TokenFilterBase and TokenizerBase descendants should use the Stringified marker.

Related to elastic/elasticsearch-net#7870

@flobernd
Copy link
Member Author

flobernd commented Aug 3, 2023

@swallez This might need further discussion as it does not seem right to use Stringified<T> for all properties.

@swallez
Copy link
Member

swallez commented Aug 15, 2023

After careful examination of the spec, it appears that most of the use of Stringified is for types represented as a Settings object in the ES server code, which is a Map<String, Object> where the Object can be either a string or child object map.

So we can introduce a new ValuesAsStrings behavior, that will ensure that all field in a class defined using their semantic type (e.g. integer or bool) but will be accepted as string values in the JSON.

We should however keep Stringified for now, as we also have some _something_ | string field definitions that could make used of Stringified without the entire class having values as strings.

As introducing a new behavior requires updates to all code generators, let's follow these steps:

  • merge this PR
  • define ValuesAsStrings in a separate PR
  • create a team-wide issue to update code generators (for languages where it's relevant)
  • and then update the spec to use ValuesAsString

@flobernd
Copy link
Member Author

@swallez From my side, we can as well abandon this PR completely in favor of the one that introduces ValuesAsStrings.

@swallez
Copy link
Member

swallez commented Aug 15, 2023

@flobernd Implementing the new behavior in code generators will take a bit of time. So it may be better to merge this PR to resolve elastic/elasticsearch-net#7870 in a timely manner, even if we will remove these new Stringified later. I'll let you decide 😉

I've opened #2238 for the new behavior (and changed its name to be consistent with Stringified).

@flobernd flobernd marked this pull request as ready for review August 15, 2023 15:44
@flobernd flobernd merged commit ab9b196 into main Aug 15, 2023
@flobernd flobernd deleted the template-stringify branch August 15, 2023 15:45
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.

2 participants