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

Removed forName method #689

Conversation

martin-morek
Copy link
Contributor

  • fixes 686 and 685
  • Removed use of forName method in push activation state machine implementation and replaced by switch constructing Event object based on name

Copy link
Contributor

@QuintinWillison QuintinWillison left a comment

Choose a reason for hiding this comment

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

Thanks, this is a great start. However, as discussed in our call earlier today, this only presents one side of the story... I think we need to see:

  • Addition of functionality that does directly the opposite of your constructEventByName static method - probably an instance method on Event, perhaps simply something like getEventName()
  • Some unit tests for this class hierarchy proving:
    • that Event instances can be represented as string and then rehydrated from the same
    • that attempts to rehydrate unsupported event types (those with non-nullary constructor) throw the right exception
    • that attempts to rehydrate unknown event types throw the right exception

We need to get rid of any code that uses runtime reflection. This has to be checked by the compiler.

@martin-morek
Copy link
Contributor Author

@QuintinWillison could you please elaborate on getEventName() method? Right now for getting event name we are using getClass().getName(), I'm not sure if this can be simplified. What we can do is define property called name for an event object and manually fill it with a value. That way when we change class name code stays functional but real name would differ.

@QuintinWillison
Copy link
Contributor

Hi @martin-morek - so, the use of getClass().getName() is the crux of the issue! 😄

That method is dynamic at runtime. If the class name is different, because of obfuscation or something else outside of control as SDK developers, then that can mess things up - in particular from build to build of a application.

Each class mast vend its own serialisation name from a static string. These cannot be dynamically sourced like this.

@QuintinWillison QuintinWillison merged commit f46272d into main Aug 5, 2021
@QuintinWillison QuintinWillison deleted the bug-686-remove-use-of-forClass-method-push-activation-state-machine branch August 5, 2021 07:52
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.

Remove use of forClass method in push activation state machine implementation
2 participants