CyberfusionIO / cyberfusion-cluster-api-client

Cyberfusion Core API client
https://core-api.cyberfusion.io
MIT License
5 stars 3 forks source link

Fix filterFields for when no specific fields are passed #35

Closed mbardelmeijer closed 1 year ago

mbardelmeijer commented 1 year ago

Changes

This PR is open for discussion. We're running into an issue with the Authentication -> login method. This method uses the filterFields method without specific fields defined. Passing the empty array to the Arr::only method will return no fields at all, which returns in authentication failure. This PR fixes this behaviour, but unsure if this is the best way to fix this.

Checks

dvdheiden commented 1 year ago

Thank you for the PR.

Actually, we could just remove the filterFields call from the login endpoint as all fields are allowed to be passed. I'm not sure if it's the best approach to use array_filter as that filters nullable fields (as we now have several nullable fields that might cause some problems), maybe when no fields are provided we should just return the source array as-is.

Besides that, I'm actually not that happy about the filterFields anyway, as it resulted in other errors before so I might give it a try to handle that differently.

dvdheiden commented 1 year ago

Did some checks, but this introduces another problem because it now filters fields that are set to null. That will reintroduce the issues with the rabbitmq_* fields. I will create a new PR for this to fix the login endpoint and will keep this in mind while working on https://github.com/CyberfusionNL/cyberfusion-cluster-api-client/issues/7.

dvdheiden commented 1 year ago

Thanks again for reporting this issue and putting the effort in to submit a PR.

This issue is fixed in https://github.com/CyberfusionNL/cyberfusion-cluster-api-client/releases/tag/v1.86.3 by removing the filterFields call in the login endpoint.

In the PR I provided an example: https://github.com/CyberfusionNL/cyberfusion-cluster-api-client/pull/36#issuecomment-1368245695

If you have any issues with the client, please let me know!

mbardelmeijer commented 1 year ago

Thanks a lot! Doubted it as well if this was the complete fix; agreed that removing the filterFields from Login makes the most sense.

Happy new year! 🎆

dvdheiden commented 1 year ago

Happy new year! 🎊🎆