Added HLRC support for PinnedQueryBuilder

[markharwood]

Added new PinnedQueryBuilder in xpack core and related test in SearchIT

Related #44074

[elasticmachine]

Pinging @elastic/es-core-features

[elasticmachine]

Pinging @elastic/es-search

[hub-cap]

LGTM from where it should live, but I dont know much about the internals of Pinned Queries so i cant speak to that

[jimczi]

If you put the query builder in xpack-core you could probably have a base class that you extend in the business-rules module to provide the toQuery implementation ? This would avoid the serialization duplication ? I also wonder if we should have a note in the HRLC documentation to mention that xpack-core is needed to build PinnedQuery ?

[markharwood]

you could probably have a base class that you extend in the business-rules module to provide the toQuery implementation ?

That was something I coded up in my original PR for PinnedQuery but we concluded subclassing was not the way forward

I also wonder if we should have a note in the HRLC documentation to mention that xpack-core is needed to build PinnedQuery ?

In this PR or proposing a different PR that describes HLRC dependencies more generally?

[jimczi]

That was something I coded up in my original PR for PinnedQuery but we concluded subclassing was not the way forward

I thought it was because we wanted to put the base class in hlrc which would be weird since in this case the xpack module would need to add a dependency on the client. In this pr the class is in xpack core so it feels more natural since the business rules module already have a dependency on xpack-core. I don't have strong feelings about this though so feel free to ignore if you think that it's not a good option.

In this PR or proposing a different PR that describes HLRC dependencies more generally?

I think we can add a small sentence in the query builders docs explaining how to use this extra query. We can revisit this section later as we add more queries there ?

[markharwood]

I thought it was because we wanted to put the base class in hlrc which would be weird

If I understand @javanna 's explanation correctly I think it was more about allowing some divergence in class versions used between client and server to allow for less brittle dependencies.

We can revisit this section later as we add more queries there ?

OK

[markharwood]

Good to go, Jim?

[javanna]

My main concern was that server should not depend on HLRC like it was initially proposed. If the base class is in x-pack-core, or any library shared between client and server, I am fine with that. Generally though, we have found that server needs one side (parse requests, print out responses) while client needs the other side (print out requests, parse back responses). Is the doXContent method needed in the server-side code? That is the only duplicated bit at the moment?

[markharwood]

Is the doXContent method needed in the server-side code?

Unfortunately I think the test infrastructure in AbstractQueryTestCase assumes the QueryBuilder class under test is multi-purpose and can round-trip all the various ways - to/from xcontent and stream input. I'm having a hard time figuring out how to satisfy all the various compiler and test constraints again that allowed me to subclass.

[javanna]

I don't think it's a big deal if doXContent is the only duplicated method, maybe leave a comment that it's duplicated for tests only? I wonder if subclassing is worth it then, probably not?

[markharwood]

I wonder if subclassing is worth it then, probably not?

I'm not sure it is. The criteria isn't complex - an array of IDs and another QueryBuilder.

[jimczi]

LGTM