feat: use Editable when possible instead of reflection to infer builder

[metacosm]

• feat: use Editable when possible instead of reflection to infer builder

[shawkins]

I think the only reason this was not done initially is that there's no guarentee that custom resources implement Editable. If a user still wants client logic that interacts with visitors, they will need to make it Editable.

There is also BaseFluent.builderOf that falls back to reflection as well (we could be calling that method here).

[sonarcloud]

Quality Gate passed

Issues
0 New issues

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

See analysis details on SonarCloud

[manusa]

I think the only reason this was not done initially is that there's no guarentee that custom resources implement Editable. If a user still wants client logic that interacts with visitors, they will need to make it Editable.

For this case maybe we can rely on a fallback?
I can't recall if users had to do additional setup to register the custom resource types for reflection. If that was the case, then this would be compliant with the current behavior, since those builders should need registering too.

[metacosm]

I think the only reason this was not done initially is that there's no guarentee that custom resources implement Editable. If a user still wants client logic that interacts with visitors, they will need to make it Editable.

Indeed, and I think that's a fair assumption to make. The current implementation attempts to locate a builder via reflection even if it doesn't exist, making users explicitly implement Editable will make them create the associated Builder if they want their custom resources to be editable.

There is also BaseFluent.builderOf that falls back to reflection as well (we could be calling that method here).

Yes, but the goal is to remove reflection altogether here.

[metacosm]

I think the only reason this was not done initially is that there's no guarentee that custom resources implement Editable. If a user still wants client logic that interacts with visitors, they will need to make it Editable.

For this case maybe we can rely on a fallback? I can't recall if users had to do additional setup to register the custom resource types for reflection. If that was the case, then this would be compliant with the current behavior, since those builders should need registering too.

Historically, QOSDK was taking care of registering Custom resources associated types for reflection, now the kubernetes client extension does it in a more complete way (though this is currently only in 3.8.0.CR1, iirc).

As far as fallback, I don't really see the point, to be honest, apart from potential backwards compatibility (but that would be in user-provided code since resources provided by the client should already be implemented to support Editable). Either resources have an associated builder and in this case it's trivial to implement Editable or they don't and no fallback will solve the issue. On the other hand, any fallback implementation would require reflection, which we're trying to remove.

For that matter, we might want to consider a Listable interface to provide a way to instantiate the associated list type without requiring reflection as well.

[shawkins]

As long as we're ok with a small breaking change, then I'm fine with trying to remove reflection. However I don't think this change alone is enough. We do transitively call the basefluent method as well. The usage of reflection there could similarly be removed and an exception thrown instead - now that our stuff is editable we won't hit the problem reflection was added to solve, but it could still be an issue for custom stuff. As for additional discovery methods I'm all for that. We had briefly kicked around directly using jandex for all of this instead, I even had a prototype, but it's fine to allow quarks to override.

…

On Fri, Feb 16, 2024, 11:16 AM Chris Laprun ***@***.***> wrote: I think the only reason this was not done initially is that there's no guarentee that custom resources implement Editable. If a user still wants client logic that interacts with visitors, they will need to make it Editable. For this case maybe we can rely on a fallback? I can't recall if users had to do additional setup to register the custom resource types for reflection. If that was the case, then this would be compliant with the current behavior, since those builders should need registering too. Historically, QOSDK was taking care of registering Custom resources associated types for reflection, now the kubernetes client extension does it in a more complete way (though this is currently only in 3.8.0.CR1, iirc). As far as fallback, I don't really see the point, to be honest, apart from potential backwards compatibility (but that would be in user-provided code since resources provided by the client should already be implemented to support Editable). Either resources have an associated builder and in this case it's trivial to implement Editable or they don't and no fallback will solve the issue. On the other hand, any fallback implementation would require reflection, which we're trying to remove. For that matter, we might want to consider a Listable interface to provide a way to instantiate the associated list type without requiring reflection as well. — Reply to this email directly, view it on GitHub <#5756 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAS4NFPK2WJ6XW26OAQBO6TYT6A6BAVCNFSM6AAAAABDMALZUOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNBYG43TQNZUGI> . You are receiving this because your review was requested.Message ID: ***@***.***>

[shawkins]

To add more on this the sundio basefluent.builderOf method did not change significantly from the past - it was always using reflection. The change I'm think of was to have the fluent.builder methods call basefluent.builderOf.

So we may not want to change builderOf, and instead introduce a new method that just checks for editable, which would be used by both the fluent builder methods and by the fabric8 client.

[andreaTP]

Probably, we need to solve #5878 along with this.