Always return an object from API methods and deprecate all existing API methods · Pull Request #1054 · zonemaster/zonemaster-backend

ghost · 2022-10-12T15:23:00Z

Purpose

The current API methods return different object types in the "result" property. In #790 it has been discussed the need to always return an object to facilitate expanding the results. It was also noted that the API methods' names could be harmonized and improved. This first work is about providing new API methods based on the discussion in #790, and deprecate all existing API methods.

This is the first step of the process:

1. create new methods and deprecate the old ones
2. update code and testcases to use the new methods (done in a follow up PR, see RPCAPI apply new methods in code, scripts and tests #1083)

Mapping between old and new method names:

current method name	current top-level property	new method name	new top-level property
`version_info`	object	`system_versions`	object
`profile_names`	array	`conf_profiles`	object
`get_language_tags`	array	`conf_languages`	object
`get_host_by_name`	array	`lookup_address_records`	object
`get_data_from_parent_zone`	object	`lookup_delegation_data`	object
`start_domain_test`	integer	`job_create`	object
`test_progress`	integer	`job_status`	object
`get_test_results`	object	`job_results`	object
`get_test_params`	object	`job_params`	object
`get_test_history`	array	`domain_history`	object
`add_api_user`	integer	`user_create`	object
`add_batch_job`	integer	`batch_create`	object
`get_batch_job_result`	object	`batch_status`	object

Context

Partially addresses #790.

Changes

Deprecate all existing API methods, see the array in the Purpose section for the new method names.

How to test this PR

Test should pass. At this stage the new methods are not used.

matsduf · 2022-10-12T16:14:43Z

I do think that the new names are better than the old ones, but is it worth the change to get better names?

#790 suggests that it should be possible to send deprecation messages with the result message, and that can sound appealing but would that really work? Who would read those messages? Do you envision that those messages are shown to the Zonemaster GUI users? Or how should those messages go to some maintainer?

ghost · 2022-10-13T10:16:07Z

I do think that the new names are better than the old ones, but is it worth the change to get better names?

Do we think this is a change we'd like to see at some point? Do we want to harmonize the API? Do we want to clean our codebase? I would answer yes to all.
Furthermore one of the point raised in #790 is our use of the "test" term that refers to several different things (a test case, a test of a zone…) , making it easy to mix things up.
I think it's important to keep our API and code base harmonized. Currently there is a lot of room for improvements (see #791 for example).

matsduf · 2023-02-09T16:05:13Z

job_progress returns the status of ongoing job or completed job.
batch_status returns the status of ongoing batch or completed batch.

job_progress and batch_progress or job_status and batch_status?

ghost · 2023-02-09T18:24:40Z

Good point, I like *_status, it's more generic. ~~I'll provide an update.~~ I've rebased and updated.

matsduf · 2023-02-14T22:08:33Z

E.g. method start_domain_test is deprecated, but it is also used in some t files (e.g. t/lifecycle.t). Other deprecated methods might be used in the t files. The new methods are not added, which I think they should be. The deprecated methods could maybe be removed now, but it would be better if they are kept until they are removed.

Else this looks good.

ghost · 2023-02-15T08:03:02Z

I planned to do this on a second stage, ie in a follow-up PR. If this is ok, could you approve this PR?

matsduf · 2023-02-15T08:59:40Z

I planned to do this on a second stage, ie in a follow-up PR. If this is ok, could you approve this PR?

That is also fine.

tgreenx

The renaming of methods is clear and appreciated. A few comments; other than that, LGTM.

tgreenx · 2023-02-22T14:48:35Z

+    connect "system_version" => {
+        handler => $handler,
+        action => "system_version"


"system_version" -> "system_versions"
(two occurrences)

tgreenx · 2023-02-22T14:48:47Z

+  * [API method: conf_profiles](#api-method-conf_profiles)
+  * [API method: conf_languages](#api-method-conf_languages)
+  * [API method: lookup_address_records](#api-method-lookup_address_records)
+  * [API method: lookup_delegatio_data](#api-method-lookup_delegation_data)


lookup_delegatio_data -> lookup_delegation_data

tgreenx · 2023-02-22T14:52:39Z

+    };
 };

 if ($config->RPCAPI_enable_add_api_user) {


Shouldn't this PR also introduce the new appropriate, unused config parameter RPCAPI_enable_user_create ?

good point, I've created this new config parameter and marked old one as deprecated

tgreenx · 2023-02-22T14:53:06Z

+    });
 }

 if ($config->RPCAPI_enable_add_batch_job) {


Shouldn't this PR also introduce the new appropriate, unused config parameter RPCAPI_enable_batch_create ?

good point, I've created this new config parameter and marked old one as deprecated

And mark old ones as deprecated

Some old API methods were already returning an object. This object can be returned.

* Document new API methods * Mark old API methods as deprecated * Link to new API methods from deprecated one * Update return values for new API methods

* create "RPCAPI.enable_batch_create" and "RPCAPI.enable_user_create" * mark "RPCAPI.enable_add_batch_job" and "RPCAPI.enable_add_api_user" as deprecated

matsduf

This is still fine.

marc-vanderwal · 2023-05-30T09:51:21Z

Release test successful.

I found one minor latent issue, #1105. But it’s very minor (it’s only visible by manually poking at the API) and seems more like an old oversight. It is not a regression introduced by this pull request, so this issue should not prevent the upcoming release from taking place.

ghost requested a review from mattias-p October 12, 2022 15:23

matsduf reviewed Oct 13, 2022

View reviewed changes

Comment thread docs/API.md Outdated

Comment thread docs/API.md Outdated

Comment thread docs/API.md

ghost marked this pull request as ready for review February 9, 2023 12:51

ghost requested review from hannaeko, marc-vanderwal and tgreenx February 9, 2023 12:51

ghost added the V-Minor Versioning: The change gives an update of minor in version. label Feb 9, 2023

ghost added this to the v2023.1 milestone Feb 9, 2023

ghost requested a review from matsduf February 9, 2023 12:52

matsduf requested changes Feb 9, 2023

View reviewed changes

Comment thread docs/API.md Outdated

Comment thread docs/API.md Outdated

Comment thread docs/API.md Outdated

ghost requested a review from matsduf February 10, 2023 14:49

ghost changed the title ~~Always return an object from API methods + deprecate and replace all existing API methods~~ Always return an object from API methods and deprecate all existing API methods Feb 15, 2023

matsduf previously approved these changes Feb 15, 2023

View reviewed changes

tgreenx reviewed Feb 22, 2023

View reviewed changes

Alexandre Pion added 4 commits February 22, 2023 17:49

Mark all API methods as deprecated

5169feb

Provide new API methods

2e8ae15

Duplicate methods with new names

cc4d7eb

And mark old ones as deprecated

Refactor: reuse old API methods

c619bec

Some old API methods were already returning an object. This object can be returned.

ghost dismissed matsduf’s stale review via cdb6c88 February 22, 2023 17:24

ghost commented Feb 22, 2023

View reviewed changes

Comment thread lib/Zonemaster/Backend/RPCAPI.pm Outdated

Alexandre Pion added 3 commits February 22, 2023 18:56

Always return an object in new API methods

aea64a8

Reuse new API methods for deprecate one

4dd6d09

Adapt documentation

af0f098

* Document new API methods * Mark old API methods as deprecated * Link to new API methods from deprecated one * Update return values for new API methods

Alexandre Pion added 5 commits February 22, 2023 18:59

Document missing output

3ccae11

Document deprecation removal time

51de625

Reset some heading level

53501b4

Clarification for deprecated methods

720a6eb

New configuration parameter in RPCAPI section

64d457f

* create "RPCAPI.enable_batch_create" and "RPCAPI.enable_user_create" * mark "RPCAPI.enable_add_batch_job" and "RPCAPI.enable_add_api_user" as deprecated

matsduf approved these changes Feb 23, 2023

View reviewed changes

tgreenx approved these changes Feb 23, 2023

View reviewed changes

ghost merged commit c6bbe44 into zonemaster:develop Feb 24, 2023

ghost mentioned this pull request Feb 24, 2023

RPCAPI apply new methods in code, scripts and tests #1083

Merged

ghost deleted the rpcapi-deprecation-warnings-1 branch February 24, 2023 10:38

ghost mentioned this pull request Feb 27, 2023

Disambiguate usage of the term "test" by using "job" #1084

Closed

ghost mentioned this pull request Mar 13, 2023

Uniform use of job #1090

Closed

marc-vanderwal added the S-ReleaseTested Status: The PR has been successfully tested in release testing label May 30, 2023

mattias-p mentioned this pull request Jun 5, 2023

Postpone stabilization of new RPC API methods zonemaster/zonemaster#1159

Closed

tgreenx mentioned this pull request Jun 7, 2023

Reinstate old RPC API methods #1111

Merged

ghost mentioned this pull request Jun 21, 2023

Preparation for release v2023.1 (Backend) #1112

Merged

mattias-p mentioned this pull request Jul 8, 2023

Stabilize new RPC API zonemaster/zonemaster#1175

Open

mattias-p mentioned this pull request Feb 6, 2024

RPCAPI deprecation warnings #790

Closed

This pull request was closed.

Uh oh!

Conversation

ghost commented Oct 12, 2022 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Context

Changes

How to test this PR

Uh oh!

matsduf commented Oct 12, 2022

Uh oh!

ghost commented Oct 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsduf commented Feb 9, 2023

Uh oh!

ghost commented Feb 9, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

matsduf commented Feb 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Feb 15, 2023

Uh oh!

matsduf commented Feb 15, 2023

Uh oh!

tgreenx left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgreenx Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

tgreenx Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

tgreenx Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Feb 22, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgreenx Feb 22, 2023

Choose a reason for hiding this comment

Uh oh!

ghost Feb 22, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

matsduf left a comment

Choose a reason for hiding this comment

Uh oh!

marc-vanderwal commented May 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost commented Oct 12, 2022 •

edited by ghost

Loading

ghost commented Feb 9, 2023 •

edited by ghost

Loading

matsduf commented Feb 14, 2023 •

edited

Loading

tgreenx left a comment •

edited

Loading

ghost Feb 22, 2023 •

edited by ghost

Loading

ghost Feb 22, 2023 •

edited by ghost

Loading