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

Add NotImplemented exception #494

Merged
merged 24 commits into from
Nov 3, 2023

Conversation

drc38
Copy link
Contributor

@drc38 drc38 commented Oct 19, 2023

See #426 for background discussion

@drc38 drc38 requested a review from OrangeTux as a code owner October 19, 2023 09:58
@drc38
Copy link
Contributor Author

drc38 commented Oct 19, 2023

Any suggestions on how to fix the circular import? Or will the _handle_call method need to be overridden in each version?

@Jared-Newell-Mobility
Copy link
Contributor

Jared-Newell-Mobility commented Oct 19, 2023

Linked issue / enhancement - #493

@OrangeTux
Copy link
Contributor

You could move the imports of the Action enums to appear after the ChargePoint class. Not sure if that's the nicest solution, though.

Copy link
Contributor

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

Can you cover the changes by unit tests?

ocpp/charge_point.py Outdated Show resolved Hide resolved
@drc38
Copy link
Contributor Author

drc38 commented Oct 19, 2023

@OrangeTux, not sure why the mock is not being called to test an invalid action. Appreciate your assistance.

Got it sorted, ready for your final review.

@drc38 drc38 requested a review from OrangeTux October 20, 2023 00:58
@drc38
Copy link
Contributor Author

drc38 commented Oct 30, 2023

I notice messages.py can raise a NotImplementedError when validating the message content, imo this is incorrect and should be a ValidationError?

@drc38
Copy link
Contributor Author

drc38 commented Oct 30, 2023

@OrangeTux @Jared-Newell-Mobility, Suggest amending to the following:

except OSError:
        raise ValidationError(
            details={"cause": f"JSON validation schema not found for action: {message.action}"}
        )
except json.JSONDecodeError:
       raise ValidationError(
            details={"cause": f"Error in decoding JSON validation schema for action: {message.action}"}
       )

Let me know if you're happy with the existing changes proposed and whether to include this in the same PR or separately, if agreed to.

Copy link
Contributor

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

Almost there!

tests/v16/conftest.py Outdated Show resolved Hide resolved
ocpp/charge_point.py Outdated Show resolved Hide resolved
@OrangeTux
Copy link
Contributor

@drc38 Are you referring to this?

ocpp/ocpp/messages.py

Lines 220 to 223 in e452f5c

except (OSError, json.JSONDecodeError):
raise NotImplementedError(
details={"cause": f"Failed to validate action: {message.action}"}
)

When a schema doesn't exists, an OSError is raised. This mechanism is (mis)used to identify calls for non existing Actions. I think a NotSupportedError should be raised in that case.

If a file is found, but it can't be parsed as JSON a json.JSONDecoder error is raised. I agree with you that this error should be mapped to a ValidationError.

Let's fix that in a seperate PR.

@drc38
Copy link
Contributor Author

drc38 commented Oct 31, 2023

@drc38 Are you referring to this?

ocpp/ocpp/messages.py

Lines 220 to 223 in e452f5c

except (OSError, json.JSONDecodeError):
raise NotImplementedError(
details={"cause": f"Failed to validate action: {message.action}"}
)

When a schema doesn't exists, an OSError is raised. This mechanism is (mis)used to identify calls for non existing Actions. I think a NotSupportedError should be raised in that case.

If a file is found, but it can't be parsed as JSON a json.JSONDecoder error is raised. I agree with you that this error should be mapped to a ValidationError.

Let's fix that in a seperate PR.

Ok. Note with this PR the OSError should only be raised on edge cases eg if the json schema file is corrupt. NotImplementedError and NotSupportedError errors will be raised prior to the json validation occurring, taking care of when the schema doesn't exist. Or have I missed something?

@drc38 drc38 requested a review from OrangeTux October 31, 2023 22:14
Copy link
Contributor

@OrangeTux OrangeTux left a comment

Choose a reason for hiding this comment

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

Thanks!

@OrangeTux OrangeTux merged commit f159cf7 into mobilityhouse:master Nov 3, 2023
6 checks passed
@drc38 drc38 deleted the add_NotImplemented branch November 3, 2023 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants