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

API errors now return information via gRPC metadata (not the response) #692

Merged
merged 13 commits into from
Aug 11, 2022

Conversation

dmondrik
Copy link
Contributor

@dmondrik dmondrik commented Aug 4, 2022

What does this Pull Request accomplish?

API errors are now returned as part of gRPC metadata rather than as part of the response. This is more natural for gRPC.

  • The approach relies on custom methods that are per resource type, since in at least some cases the message comes dynamically from the resource handle (session).
    • Ideally the names of those methods could be simple and rely on parameter overloading to resolve the difference. However, XNET has 2 resources types that are both 32-bit integers. Therefore the simplest solution is to include the resource type name as part of the method name.
  • The methods should be hand maintained because many of them require a fallback process. In MI for example, it returns the last error code for the session as well as the description. However, that may not be the status code we wanted to look up (that can happen if 2 threads are called in parallel and both fail with different status codes.)
    • In that case, such drivers have a different method of looking up a static error message for a given error code. The name and signature of that fallback method varies greatly.

Why should this Pull Request be merged?

Task 2103382: Change generated code to return UNKNOWN on error status.

What testing has been done?

I built locally. I have not run any tests manually but I am expecting they will run as part of this PR.

@dmondrik dmondrik requested a review from DavidCurtiss August 4, 2022 22:57
@dmondrik dmondrik changed the title API errors now use custom helper methods ConvertApiErrorStatusFor* to return a message API errors now return error information via gRPC metadata rather than the response. Aug 4, 2022
@dmondrik dmondrik changed the title API errors now return error information via gRPC metadata rather than the response. API errors now return information via gRPC metadata rather than the response. Aug 4, 2022
@dmondrik dmondrik changed the title API errors now return information via gRPC metadata rather than the response. API errors now return information via gRPC metadata (not the response) Aug 4, 2022
source/server/converters.h Outdated Show resolved Hide resolved
source/server/converters.h Outdated Show resolved Hide resolved
source/custom/nidaqmx_service.custom.cpp Outdated Show resolved Hide resolved
source/custom/nidcpower_service.custom.cpp Outdated Show resolved Hide resolved
source/custom/nifgen_service.custom.cpp Outdated Show resolved Hide resolved
source/codegen/templates/service_helpers.mako Show resolved Hide resolved
source/codegen/templates/service_helpers.mako Show resolved Hide resolved
source/codegen/templates/service_helpers.mako Outdated Show resolved Hide resolved
source/codegen/templates/service_helpers.mako Outdated Show resolved Hide resolved
source/codegen/templates/service_helpers.mako Outdated Show resolved Hide resolved
source/codegen/templates/service_helpers.mako Outdated Show resolved Hide resolved
source/server/converters.h Outdated Show resolved Hide resolved
source/custom/nidcpower_service.custom.cpp Outdated Show resolved Hide resolved
source/server/converters.h Outdated Show resolved Hide resolved
Copy link
Contributor

@SparkingSpork SparkingSpork left a comment

Choose a reason for hiding this comment

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

Approving based on CMakeLists changes and deferring to others on the code generation/API changes.

CMakeLists.txt Outdated Show resolved Hide resolved
@reckenro
Copy link
Collaborator

reckenro commented Aug 5, 2022

Looks like there's four integration tests (run as part of build_desktop part of CI) expecting errors that'll need to be updated to fit the new expectations:

NiFakeNonIviServiceTests.InitWithReturnedSessionFailsInit_AccessSession_SessionIsNotInMapAndStatusValueIsReturned
NiFakeNonIviServiceTests.GetStringAsReturnedValueReturnsNull_ReturnsErrorAndEmptryString
NiFakeNonIviServiceTests.InitWithError_CallsGetLatestErrorAndReturnsMessage
NiFakeServiceTests.NiFakeService_FunctionCallErrors_ResponseValuesNotSet

I'm expecting a good bit of System tests to start failing too that expect and check errors (run after a successful build_desktop action is run).

source/server/converters.h Outdated Show resolved Hide resolved
source/server/converters.h Outdated Show resolved Hide resolved
source/server/converters.h Outdated Show resolved Hide resolved
@dmondrik
Copy link
Contributor Author

dmondrik commented Aug 8, 2022

Looks like there's four integration tests (run as part of build_desktop part of CI) expecting errors that'll need to be updated to fit the new expectations:

NiFakeNonIviServiceTests.InitWithReturnedSessionFailsInit_AccessSession_SessionIsNotInMapAndStatusValueIsReturned NiFakeNonIviServiceTests.GetStringAsReturnedValueReturnsNull_ReturnsErrorAndEmptryString NiFakeNonIviServiceTests.InitWithError_CallsGetLatestErrorAndReturnsMessage NiFakeServiceTests.NiFakeService_FunctionCallErrors_ResponseValuesNotSet

I'm expecting a good bit of System tests to start failing too that expect and check errors (run after a successful build_desktop action is run).

I'll be waiting for that now that I've fixed up the unit tests.

@dmondrik dmondrik merged commit 13aca5b into main Aug 11, 2022
@dmondrik dmondrik deleted the users/danrm/api-errors branch August 11, 2022 15:29
@reckenro reckenro added the binary-breaking Change to proto file that requires client updates label Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
binary-breaking Change to proto file that requires client updates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants