Python ServiceDescriptor.FindMethodByName raises exception (inconsistent behavior between C extension and native Python + documentation)

[tomerv]

What version of protobuf and what language are you using?
Version: master
Language: Python

What operating system (Linux, Windows, ...) and version?
Ubuntu 20

What runtime / compiler are you using (e.g., python version or gcc version)
Python 3.8.10

What did you do?

service_desc = desc_pool.FindServiceByName(service_name)
service_desc.FindMethodByName(_INVALID_SYMBOL_NAME)

This raises a KeyError exception.
However, the documentation says:

Returns: the descriptor for the requested method, if found.
Return type: MethodDescriptor or None

More specifically, the native Python implementation & documentation is:

  def FindMethodByName(self, name):
    """Searches for the specified method, and returns its descriptor.
    Args:
      name (str): Name of the method.
    Returns:
      MethodDescriptor or None: the descriptor for the requested method, if
      found.
    """
    return self.methods_by_name.get(name, None)

While the C-extension implementation is:

static PyObject* FindMethodByName(PyBaseDescriptor *self, PyObject* arg) {
  Py_ssize_t name_size;
  char* name;
  if (PyString_AsStringAndSize(arg, &name, &name_size) < 0) {
    return nullptr;
  }

  const MethodDescriptor* method_descriptor =
      _GetDescriptor(self)->FindMethodByName(StringParam(name, name_size));
  if (method_descriptor == nullptr) {
    PyErr_Format(PyExc_KeyError, "Couldn't find method %.200s", name);
    return nullptr;
  }

  return PyMethodDescriptor_FromDescriptor(method_descriptor);
}

Note the PyExc_KeyError part in this code.

What did you expect to see
Consistency between Python and C-extension implementation (and the documentation).

What did you see instead?
They are not consistent.

Despite the title, I tend to think that the exception is the more correct behavior, based on these aspects:

• It feels more Pythonic.
• DescriptorPool.FindMethodByName raises a KeyError in this case. There are also tests for this behavior python/google/protobuf/internal/descriptor_pool_test.py, while there are no corresponding tests for ServiceDescriptor.FindMethodByName.
• Fixing this inconsistency will be a breaking change for either the C-extension or Python users, depending on the fix. I assume that most environments are using the C-extension version, so changing to an exception will be less harmful.

I can try to submit a patch, but I want to check first that this is the correct direction, i.e. change the native Python code to raise an exception, update the documentation accordingly, and add an appropriate test.

[tomerv]

@anandolee - please see my comment above

I can try to submit a patch, but I want to check first that this is the correct direction, i.e. change the native Python code to raise an exception, update the documentation accordingly, and add an appropriate test.

I'm willing to do the legwork to fix this, but I want to check first that this is the right direction.
Thanks.

[anandolee]

change pure python to raise key error is right direction. Thank you tomerv

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please add a comment.

This issue is labeled inactive because the last activity was over 90 days ago.

[github-actions]

We triage inactive PRs and issues in order to make it easier to find active work. If this issue should remain active or becomes active again, please reopen it.

This issue was closed and archived because there has been no new activity in the 14 days since the inactive label was added.