Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proof of concept: MastodonMinServerVersion annotation #335

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

PattaFeuFeu
Copy link
Collaborator

Description

Our Wiki mentions:

All methods are annotated using a @MastodonMinServerVersion runtime annotation. The goal of this annotation is to specify the minimum server version that is required in order to make this call successful. During library initialization, we fetch the server version from the instance. When the method is invoked, we compare versions and if requirements are not met, we will throw a BigBoneVersionException. The caller can then decide how to proceed further (e.g. call another method or fail).

So far, that was not at all the case. But I like the idea, so I wanted to implement a proof of concept which is how this PR should be seen: Not a mergeable change set, but grounds for discussion.

I have added the annotation and additional functions to check if a called method’s min version is higher than or equal to the known instance version.

I’m currently not too happy with the InstanceMethods::getInstance.requireMinVersion(client) call and that would be my main criticism for the current solution. My knowledge regarding annotations, reflection, or annotation-based code is very much in its infancy so I’m sure there are better solutions that I’d still need to learn about. ksp might be an option to auto-create the boilerplate code so that we only need to add the annotation and then ksp handles the rest for us and automatically adds code similar to what I’ve added for getInstance as an example for now.

Before I continue with this, I’d like to find out what you think of this. Is this a good way forward? Would you like a different approach?

Type of Change

(Keep the one that applies, remove the rest)

  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Dependency addition

Breaking change

Methods annotated with @MastodonMinServerVersion may now throw a BigBoneVersionException which leads to errors from Java callers if they’re not explicitly handling that exception (or its parent Exception).

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Mandatory Checklist

  • My change follows the projects coding style
  • I ran gradle check and there were no errors reported
  • I have performed a self-review of my code
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • KDoc added to all public methods

@PattaFeuFeu PattaFeuFeu added enhancement New feature or request code quality Everything related to code quality breaking Incompatible with previous versions dependencies Pull requests that update a dependency file needs discussion It is not yet entirely clear on how to address this issue labels Nov 5, 2023
@PattaFeuFeu PattaFeuFeu self-assigned this Nov 5, 2023
@PattaFeuFeu PattaFeuFeu force-pushed the feature/annotation-mastodonMinServerVersion branch from 0e19827 to 29e379b Compare November 5, 2023 00:48
Comment on lines +75 to +78
"""^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)""" +
"""(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)""" +
"""(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?""" +
"""(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?${'$'}"""
Copy link
Collaborator Author

@PattaFeuFeu PattaFeuFeu Nov 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need to be a bit more lenient, seeing as Mastodon doesn’t follow semantic versioning to the letter at least when it comes to release candidates, as can be seen in this example JSON:

They only switched to a more SemVer-friendly variant starting with 4.2.0-beta3 in mastodon/mastodon#26653:

https://github.com/mastodon/mastodon/blob/v4.2.0-beta3/lib/mastodon/version.rb#L36-L38

@bocops
Copy link
Collaborator

bocops commented Nov 5, 2023

I agree with your self-criticism. Annotations are great if they help remove boilerplate code, or at least having to write it ourselves - but having to add a call to requireMinVersion() in every API method is the opposite. As long as that is necessary, we might as well just add a minMastodonVersion String as another parameter to our client.getMastodonRequest(), and then do the version check in that function. That aside, if there is a way to properly achieve this using annotations, the rest of it looks good.

The only thing I'd like to see for the moment is an option to disable version checks when building the MastodonClient (or reduce them to less than an exception), so that library users at least have the choice to use BigBone with all those platforms that offer a Mastodon-compatible API while using very different versioning, for example Pixelfed, GotoSocial, Hometown, and probably many more.

Long-term, I believe having a proper way to support different platforms would be useful (see #226).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Incompatible with previous versions code quality Everything related to code quality dependencies Pull requests that update a dependency file enhancement New feature or request needs discussion It is not yet entirely clear on how to address this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants