Query body

[shroffk]

#211

notes:

 // Backward-compatible overload when no request body is provided.
  default Scroll query(String scrollId, MultiValueMap<String, String> searchParameters) {
    return query(scrollId, searchParameters, null);
  }

this bit is primarily for the Tests and not used for anything else

Currently, I try to merge the query parameters from the URL and the request body. This is good for backward compatibility but we could simplify this by only allowing one or the other or simply prioritize one or the other.

Neither of the above points are show stoppers and I think this addition would simplify and improve performance of clients.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[shroffk]

curl --location --request GET 'http://localhost:9090/ChannelFinder/resources/channels?iocid=127.0.0.1%3A*' \
--header 'Content-Type: application/json' \
--header 'Authorization: ••••••' \
--data '{"~name":"*74*,*75*", "hostName":"LEC-175107"}

now you are no longer limited by 600 chars

[github-actions]

Overall Project	13.51% -0.47%	❌
Files changed	59.44%	❌

File	Coverage
SearchParameterMergerUtil.java	100%	🍏
ChannelController.java	0% -41.54%	❌
ChannelScrollController.java	0% -76.92%	❌
IChannel.java	0%	❌
IChannelScroll.java	0%	❌

[jacomago]

Do we really want to do this? Or make api version 2? Then we can get rid of the MultiValueMap.

[jacomago]

I think you can just do two methods rather than mutually exclusive inputs.

[shroffk]

well this is not part of the current change so I would address this as a separate issue

[jacomago]

I don't like a generic Map<String, String> better to use an actual record

record QueryOptions(String name, Map<String, String> properties, List tags) {}

You can see it makes the swagger much much nicer.

[shroffk]

Why should we do this now?

These small changes have significant impact on the performance of recceeiver.
Considering this is backward compatible I feel we should add this.

[anderslindho]

I side with @jacomago, questioning if we should do this. Even if backwards compatible it feels the wrong direction. I think we instead should be adding these capabilities to the v1 API (but then as POST under the appropriate resource).

This PR is also introducing some layer slippage: SearchParameterMergerUtil should prob go into web/v0/ rather than common/. And I am not sure about the overload as test accommodation, plus the behaviour for query param PLUS body feels a bit strange. Finally, should we really be designing around current recsync stack when we have in mind to replace it?

[shroffk]

as POST under the appropriate resource

We can add to POST under v1 along with GET ( this is mirroring the behaviour of the elastic REST API )

layer slippage: SearchParameterMergerUtil should prob go into web/v0/ rather than common/.

I thought since this is utility class, common would be the right place for it. We might have to expand it based on new requirements from v1. I am fine moving it to common.

query param PLUS body feels a bit strange.

While I don't totally disagree, in v1 I am fine with splitting these into different end points

GET/POST ...ChannelFinder/resources/channels/_search/{channelName}

GET/POST ...ChannelFinder/resources/channels/_search + body with search criteria

Finally, should we really be designing around current recsync stack when we have in mind to replace it?

These are the feature I need to complete my new implementation for recceiver. I need some of these changes to support the case of recceiver supporting millions of PV's

[shroffk]

Considering the changes are backward compatible could we merge this and the batch delete... I don't think I will need any other changes and we can start focusing on v1

[anderslindho]

I am not completely following @shroffk. I suggested POST because GET has a character limitation that makes it unfit for batch/bulk ops. I don't love the idea to mirror the ES REST API here; that would be exposing repo layer details in our API design, which is the opposite direction we want to go IMO. In fact I am not sure about the ES repo layer in CF in the first place, but that's a separate conversation.

Re recceiver2: we are 2 sites doing separate implementations and we have not in any way aligned on an improved design of either. I understand you need this for your new recceiver, but I don't think that should drive CF mainline API changes - especially not ones with known design issues. CF has its own lifecycle and multiple clients beyond recsync.

What I want us to move towards is a CF with proper clean layer separation and a truly RESTful API. Recsync is then a separate matter; it's just one way to populate CF, not the other way around.

For that reason I'd rather we hold this and do it properly in v1 as a POST endpoint under the right resource. I'm happy to prioritise that discussion if it unblocks you.

[shroffk]

that would be exposing repo layer details in our API design

I just used the elastic API as a reference/resources... maybe mirroring was not a good word. This does not bind us to es or in anyway expose the underline storage layer.

I am in agreement with POST with a query body being the preferred solution for CF v1.

However, for the time being I just need these two updates to significantly simplify my workflow. The "new" recceiver was started a few years ago so I thought it would be nice to complete it.

I agree that CF has its own life cycle, but the recceiever is by far the most important client. It is the one which will usually defines the performance and scalability boundaries of CF.

I am guessing we will not be merging this so I will update my client... are we Ok with the batch delete? #213 #214

[anderslindho]

I am guessing we will not be merging this so I will update my client... are we Ok with the batch delete? #213 #214

Will comment in that PR for traceability