Skip to content

Add number type conversion validation to ensure proper casting between numeric types#1205

Draft
mcruzdev wants to merge 2 commits intoserverlessworkflow:mainfrom
mcruzdev:issue-1200
Draft

Add number type conversion validation to ensure proper casting between numeric types#1205
mcruzdev wants to merge 2 commits intoserverlessworkflow:mainfrom
mcruzdev:issue-1200

Conversation

@mcruzdev
Copy link
Collaborator

@mcruzdev mcruzdev commented Mar 5, 2026

Changes

  • Implement convertNumber method in AbstractWorkflowModel to handle conversions between numeric types
  • Fix issue where asNumber() returned incompatible Number types without proper casting
  • Ensure type safety when converting workflow model outputs to specific numeric types

Closes #1200

@mcruzdev mcruzdev requested a review from fjtirado as a code owner March 5, 2026 15:08
Copilot AI review requested due to automatic review settings March 5, 2026 15:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a convertNumber private method to AbstractWorkflowModel to properly dispatch numeric type conversions when as(Class<T>) is called with a Number-derived class (previously returning whatever asNumber() produced, which could cause a ClassCastException at the call site). It also adds a comprehensive suite of integration tests covering several primitive wrapper conversions (Long↔Integer, Double↔Integer, Float↔Double, Short↔Integer, Byte↔Integer).

Changes:

  • AbstractWorkflowModel.java: Replace the raw asNumber() call with asNumber().map(num -> convertNumber(num, clazz)), and add the convertNumber dispatch method.
  • WorkflowNumberConversionTest.java: New integration test class exercising seven conversion scenarios.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File Description
impl/core/src/main/java/io/serverlessworkflow/impl/AbstractWorkflowModel.java Adds convertNumber method and wires it into as() for Number-typed requests
impl/test/src/test/java/io/serverlessworkflow/impl/test/WorkflowNumberConversionTest.java Integration tests for numeric type conversions across multiple primitive wrapper types

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +63 to +65
} else {
return (T) num;
}
Copy link

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

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

The convertNumber method does not handle BigDecimal or BigInteger types, even though both are Number subclasses supported by the codebase (as evidenced by JacksonModelFactory.from(Number) and JsonUtils handling them). When as(BigDecimal.class) or as(BigInteger.class) is called, execution reaches the else { return (T) num; } branch. Because asNumber() in JacksonModel always returns a Long (via node.asLong()), this unchecked cast will produce a ClassCastException at the call site. The convertNumber method should explicitly handle BigDecimal and BigInteger — for example, by calling new BigDecimal(num.toString()) or BigDecimal.valueOf(num.longValue()) for BigDecimal, and BigInteger.valueOf(num.longValue()) for BigInteger.

Copilot uses AI. Check for mistakes.
Comment on lines +50 to +65
private <T> T convertNumber(Number num, Class<T> clazz) {
if (clazz == Integer.class) {
return (T) Integer.valueOf(num.intValue());
} else if (clazz == Long.class) {
return (T) Long.valueOf(num.longValue());
} else if (clazz == Double.class) {
return (T) Double.valueOf(num.doubleValue());
} else if (clazz == Float.class) {
return (T) Float.valueOf(num.floatValue());
} else if (clazz == Short.class) {
return (T) Short.valueOf(num.shortValue());
} else if (clazz == Byte.class) {
return (T) Byte.valueOf(num.byteValue());
} else {
return (T) num;
}
Copy link
Collaborator

@fjtirado fjtirado Mar 5, 2026

Choose a reason for hiding this comment

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

I think we need to handle this differently, rather than using the asNumber public interface method, add a new protected method here asNumber(Class<?> targetNumberClass) and implement it on the children classes.

That way, you can use JsonNode.asLong, asInt methods in JakcsonModel and copy this implementation to JavaModel. The approach you took implies two conversions: the one performed by asNumber (in JavaModel is straightforward, but in Jackson it is not) and the one performed by this method. It is more efficient to have one in the children class.

@fjtirado fjtirado marked this pull request as draft March 5, 2026 16:26
…n numeric types

Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
return (Optional<T>) asDate();
} else if (Number.class.isAssignableFrom(clazz)) {
return (Optional<T>) asNumber();
return asNumber().map(num -> convertNumber(num, clazz));
Copy link
Collaborator

Choose a reason for hiding this comment

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

here is whereasNumber(class)method should be invoked

Signed-off-by: Matheus Cruz <matheuscruz.dev@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incompatible type from task's output and outputAs's input

3 participants