RPCAPI deprecation warnings

[mattias-p]

Purpose

It should be possible to report deprecation warnings back to RPCAPI clients. This would allow us to update the APIs in a friendlier fashion.

Scope

This issue is about creating a channel or making room for delivery of deprecation warnings. It is not about introducing deprecation warnings.

Design

1. For each API method that today puts a scalar or an array in its top-level "result" property:
   1. Copy this method to a different name.
   2. Change the response object of the new method so that the top-level "result" property is an object instead of a scalar or array. Let the new object have a single property with the scalar/array data as its value.
   3. Mark the original methods as deprecated in the API documentation with a note encourage clients to migrate to the new method.
2. For each method in the API, add an optional property in the top-level "result" object. Let the new property be named "warnings" and let it have a non-empty list of human readable strings as its value.

Implementation-wise you'd probably have one method call the other and just modify the result. Documentation-wise you'd probably do something similar.

It would be nice if we could have the old methods emit deprecation warnings when you use them, encouraging clients to migrate to the new method. But that's exactly the problem with those methods. We can't. And that's exactly why they're being replaced.

Alternatives

• Instead of adding a "warnings" property into the "result" object, we could add it to the top-level response object. However I don't think it's allowed in the JSON-RPC 2.0 specification, and I'm not sure we could coax JSON::RPC into doing this.
• Instead of giving immediate feedback about deprecated usage in the response object, we could introduce a new method that clients to poll to receive warnings generated in previous calls. It's not clear what mechanism we'd use to give the right deprecation warnings to the right client.
• Instead of giving immediate feedback about deprecated usage through the RPC-API, we could announce deprecations through documentation only. E.g. in the API documentation and release notes.

[ghost]

I think this would be nice to make room for such channel. With regard to your suggested design, here are the methods that need to be updated:

method	to update	top-level property
version_info		object
profile_names	yes	array
get_language_tags	yes	array
get_host_by_name	yes	array
get_data_from_parent_zone		object
start_domain_test	yes	integer
test_progress	yes	integer
get_test_results		object
get_test_history	yes	array
get_test_params		object
add_api_user	yes	integer
add_batch_job	yes	integer
get_batch_job_result		object

The suggestion is to provide a new name for the ones that need to be updated. I think we could revise the name of all methods to provide something more consistent.
I divided the methods in several groups:

• generic
• configuration related
• test related
• batch related

Then I used the group as prefix and came up with the following suggestion (- means that the method's name is unchanged):

old	new
version_info	-
profile_names	conf_get_profiles
get_language_tags	conf_get_languages
get_host_by_name	get_a_aaaa_records
get_data_from_parent_zone	-
start_domain_test	test_check
test_progress	test_get_progress
get_test_results	test_get_results
get_test_history	test_get_history
get_test_params	test_get_params
add_api_user	batch_create_user
add_batch_job	batch_create_job
get_batch_job_result	batch_get_results

What do you think?

edit: the old/new array has a new proposal, see #790 (comment)

[matsduf]

Then I used the group as prefix and came up with the following suggestion (- means that the method's name is unchanged):

I like the idea of creating more consistent naming, and I think your proposal is good. I have some suggestions for improvements.

version_info -

To be consistent with the the rest, e.g. system_get_versions.

get_host_by_name get_a_aaaa_records

I think get_address_records would be better, but to be more consistent, lookup_get_address_records or lookup_address_records.

get_data_from_parent_zone -

I think get_delegation_data would be better, but to be more consistent, lookup_get_delegation_data or lookup_delegation_data.

start_domain_test test_check

To be consistent with batch testing, test_create_job

[matsduf]

I think this would be nice to make room for such channel. With regard to your suggested design, here are the methods that need to be updated:
method to update top-level property
version_info object

If we update the API is updated I suggest that we add the Zonemaster::LDNS version to the version_info result.

[matsduf]

Alternatives

* Instead of adding a "warnings" property into the "result" object, we could add it to the top-level response object. However I don't think it's allowed in the JSON-RPC 2.0 specification, and I'm not sure we could coax [JSON::RPC](https://metacpan.org/pod/JSON::RPC) into doing this.

* Instead of giving immediate feedback about deprecated usage in the response object, we could introduce a new method that clients to poll to receive warnings generated in previous calls. It's not clear what mechanism we'd use to give the right deprecation warnings to the right client.

The user would not be happy to get warnings about deprecated API methods. I do not really see how this approach should reach the adminitrator.

* Instead of giving immediate feedback about deprecated usage through the RPC-API, we could announce deprecations through documentation _only_. E.g. in the API documentation and release notes.

I think it would make more sense to create a deprecation document and to point to that document in installation document, release notes and README of Zonemaster-Backend.

[ghost]

Updating the proposal with your feedback:

old	new
version_info	system_versions
profile_names	conf_profiles
get_language_tags	conf_languages
get_host_by_name	lookup_address_records
get_data_from_parent_zone	lookup_delegation_data
start_domain_test	test_create_job
test_progress	test_get_progress
get_test_results	test_get_results
get_test_history	test_get_history
get_test_params	test_get_params
add_api_user	batch_create_user
add_batch_job	batch_create_job
get_batch_job_result	batch_get_results

[matsduf]

Looks fine.

[mattias-p]

If we update the API is updated I suggest that we add the Zonemaster::LDNS version to the version_info result.

This makes sense to me. And also the ldns version. In an ideal world Zonemaster::LDNS would simply be a bunch of Perl bindings for ldns. In that case the version number of the bindings would be of limited use, but as things stands Zonemaster::LDNS also includes a bunch of Perl code that really ought to be moved to Zonemaster::Engine, so until we get that cleaned up we should really include the Zonemaster::LDNS version too.

The version numbers returned by version_info report what versions will be used in test jobs started in the near future. For the version numbers used by a given historical job we need to look into the test results (or some other metadata) for that specific job.
Today Engine is already emitting a DEPENDENCY_VERSION tag with the Zonemaster::LDNS version, though it's being emitted at DEBUG severity by default is DEBUG, so I guess it's filtered out before it reaches GUI.

I think we should:

1. Remove all DEPENDENCY_VERSION tags except the one for Zonemaster::LDNS. (I don't think it makes sense to include them, it's only leaking implementation details.)
2. Add a DEPENDENCY_VERSION tag for ldns itself.
3. Change the default severity of DEPENDENCY_VERSION to INFO.
4. Add properties for Zonemaster::LDNS and ldns to the version_info result.

However this could be done independently of the API change. Adding a new field to an existing object in an RPCAPI result should be considered an addition, not a breaking change. (I.e. a bump to the minor version, not to the major version.)

[mattias-p]

The user would not be happy to get warnings about deprecated API methods. I do not really see how this approach should reach the adminitrator.

Can you describe how this would negatively affect the user?

Regarding administrators we could update GUI to detect these warnings and log them. (We do have a log like this, don't we?) The administrator already has a responsibility to monitor the logs.

[mattias-p]

I really like the updated method renaming proposal! However I do have an old terminology itch that I'd like to scratch a little more here. I guess the basic goal I'm aiming for is to reduce the overloading of the term "test".

I suggest changing the "test" category to "job" and changing the "_create_job" suffix to just "_create" or to "_enqueue". A job is widely recognized as referring to a unit of work anyway. Also a "batch" would consist of "jobs" but it would not be a "job" itself, so we'd avoid overloading that word as well.

[ghost]

Updating the proposal with latest feedback:

old	new
version_info	system_versions
profile_names	conf_profiles
get_language_tags	conf_languages
get_host_by_name	lookup_address_records
get_data_from_parent_zone	lookup_delegation_data
start_domain_test	job_create
test_progress	job_progress
get_test_results	job_results
get_test_params	job_params
get_test_history	domain_history
add_api_user	user_create
add_batch_job	batch_create
get_batch_job_result	batch_results or batch_status

Using job instead of test made it clear that get_test_history is not related to a specific job/test but to a domain. So I suggested another name.

About batch_results/batch_status, rereading the API documentation, it seems that the output is not exactly about results but providing info on the batch (number of finished jobs, number of running jobs, list of finished job's id), that's why I suggest batch_status instead.

[matsduf]

The focus of Zonemaster is not domain names but tests of domain names. I think test_history is more understandable than domain_history.

I agree that batch_status is better than batch_results.

[mattias-p]

This issue was implemented in #1054. I had a look through this thread and identified a couple of followups that can be implemented separately.

• Emit warnings for deprecated usages #1149
• Dependency version messages zonemaster-engine#1320