JDBC backend search filter for listing only devices or only gateways

[gdimitropoulos-sotec]

Hono Issue for PR

Based on PR

Implement gateway filter for JDBC (H2 & Postgresql) device registry:

• add optional parameter "isGateway"
• functionality for listing only devices or gateways
• add documentation

Also-by: Matthias Feurer [email protected]
Signed-off-by: g.dimitropoulos [email protected]

[sonatype-lift]

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

[sophokles73]

Can you also please rebase the PR?

[gdimitropoulos-sotec]

The branch is rebased

[sophokles73]

hmm, GitHub still seems to believe that there are conflicts with the main branch that need to be resolved ...

[gdimitropoulos-sotec]

rebase should be ok now

[sophokles73]

@gdimitropoulos-sotec before we go on with this review: I appreciate all the effort you have put into this PR so far. However, the longer I think about the changes you have introduced, the less I am convinced that this is the right way to address the issue.

The JSON functions used in the queries in conjunction with regex based replace functions etc all indicate to me that these queries will have quite substantial execution times while Hono's protocol adapters themselves do not need any of this functionality.

I guess what I am trying to say is that I believe that without substantial changes to the underlying database schema, in particular replacing the via JSON array with a proper relation to another table, implementing this functionality will not be possible without compromising on performance and maintainability.

In addition, I am also no longer convinced that adding a separate query parameter isGateway in addition to the filterJson is the right choice. Instead, and based on the database schema change mentioned above, adding a synthetic field is-gateway in conjunction with the existing filterJson should do the trick, e.g.

{
  "field": "is-gateway",
  "op": "eq",
  "value": "true"
}

this might require special handling when parsing the query but it should make the overall user experience more convenient.

Another approach that I can think of could be to define a query view on top the device and tenant tables which could produce this synthetic property and then run the queries against the view ...

WDYT?

[julian-sotec]

@gdimitropoulos-sotec before we go on with this review: I appreciate all the effort you have put into this PR so far. However, the longer I think about the changes you have introduced, the less I am convinced that this is the right way to address the issue.

The JSON functions used in the queries in conjunction with regex based replace functions etc all indicate to me that these queries will have quite substantial execution times while Hono's protocol adapters themselves do not need any of this functionality.

I guess what I am trying to say is that I believe that without substantial changes to the underlying database schema, in particular replacing the via JSON array with a proper relation to another table, implementing this functionality will not be possible without compromising on performance and maintainability.

In addition, I am also no longer convinced that adding a separate query parameter isGateway in addition to the filterJson is the right choice. Instead, and based on the database schema change mentioned above, adding a synthetic field is-gateway in conjunction with the existing filterJson should do the trick, e.g.

{
  "field": "is-gateway",
  "op": "eq",
  "value": "true"
}

this might require special handling when parsing the query but it should make the overall user experience more convenient.

Another approach that I can think of could be to define a query view on top the device and tenant tables which could produce this synthetic property and then run the queries against the view ...

WDYT?

Hi @sophokles73: We thought of this as well and agree with you in terms of general usability. We came to the conclustion that we want to accomplish this with changing the jdbc database schema in a new issue.
However - we still think that the changes done here are valuable and improve things to the gateway feature for jdbc based use cases.
We would therefore suggest merging this PR now and taking care about the changes and your recommendations in a new issue.
See: #3568
WDYT?

[sophokles73]

@gdimitropoulos-sotec what's your opinion on this?

[gdimitropoulos-sotec]

@gdimitropoulos-sotec what's your opinion on this?

Hello @sophokles73 I agree with Julian. We have discussed this together with Julian and the team and we agree, that will be better, to have a separate table to hold the relationships between a gateway and devices (instead of the "Via" attribute).

[harism]

This implementation seems to take into consideration only the via attribute when determining normal and gateway devices. However there are also the memberOf and viaGroups attributes for device/gateway groups which are skipped altogether now if I understood correctly, should they count as normal or gateway device details too in this search? And moreover, is it so that with gateway groups one device can be both the same time; act as a gateway for some devices and be a member of some other gateway group. I'm not sure about this though.

This applies also to the via -table suggestion, should it concentrate on all device/gateway setting options instead.

[gdimitropoulos-sotec]

Hi @harism, thank you for your comment, yes we use only the via attribute to define devices and gateways. In the new issue, we will examine also the fields memberOf and viaGroups before designing and implementing the solution. Is It possible to link your comment to the new issue?

[sophokles73]

@gdimitropoulos-sotec are you working on this?

[gdimitropoulos-sotec]

@gdimitropoulos-sotec are you working on this?

Hello @sophokles73 I just resolved all comments. From our side, we wait until the branch is merged.

[sophokles73]

@gdimitropoulos-sotec sorry, but it doesn't look like you have changed any code at all. Can you double check?

[gdimitropoulos-sotec]

@gdimitropoulos-sotec sorry, but it doesn't look like you have changed any code at all. Can you double check?

Hello, I am very sorry about that. I am trying to find my commit from yesterday but I can't. I just made a new commit with all the changes.

[gdimitropoulos-sotec]

Hi @sophokles73, I fix all the issues from review comments, can this PR be merged?

[gdimitropoulos-sotec]

Hi @sophokles73 thank you for your comments. The "isGateway" is documented in both the JavaDoc and device-registry-v1.yaml file.

[gdimitropoulos-sotec]

Hi @sophokles73, I understand if the isGateway is documented. I changed the documentation so that is clear if the parameter isGateway is used then the parameter filter will be ignored.

[sophokles73]

LGTM