Skip to content

Generate a _type attribute with the target class for min, max, range etc #98

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

Merged
merged 9 commits into from
Aug 1, 2023

Conversation

rbygrave
Copy link
Contributor

We are especially interested in the target types of numbers and datetime types if we want to simplify or optimise the min, max, range, size etc adapters as they can be created knowning the specific type that is being validated.

For example:

The extra _type attributes here with int.class and long.class

  public ARangeValidationAdapter(ValidationContext ctx) {
    this.anIntValValidationAdapter = 
        ctx.<Integer>adapter(Range.class, Map.of("min",1L, "max",3L, "message","{avaje.Range.message}", "_type",int.class));

    this.aLongValValidationAdapter = 
        ctx.<Long>adapter(Range.class, Map.of("min",1L, "max",3L, "message","{avaje.Range.message}", "_type",long.class));

  }

We are especially interested in the target types of numbers and datetime types if we want to simplify or optimise the min, max, range, size etc adapters as they can be created knowning the specific type that is being validated.
@sonatype-lift
Copy link

sonatype-lift bot commented Jul 31, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

Copy link
Collaborator

@SentryMan SentryMan left a comment

Choose a reason for hiding this comment

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

I think switch is cooler, but LGTM.

@SentryMan
Copy link
Collaborator

Though hmm, I would like to see at least one test for list/map cases

@rbygrave
Copy link
Contributor Author

I have the thought of translating the Class into a "well known String" and put that into _type instead. With the String values we can then use switch.

int.class | Integer.class -> "Integer"
long.class | Long.class -> "Long"
...
BigDecimal.class -> "BigDecimal"
CharacterSequence | String.class -> "String"

... and if it isn't one of these known types leave it out. So not populating _type for List & Map etc.

@SentryMan
Copy link
Collaborator

not sure if I'm visualizing this correctly, as part of this may you modify either the time/number adapters to show what you mean?

@SentryMan
Copy link
Collaborator

I also have a question about runtime checks, suppose somebody creates something like this:

record TimeKeeper(@Past Temporal time)

for validating whether any of the java.time classes are in the past. We'd still need to check the actual type at runtime to do the validation no?

@SentryMan
Copy link
Collaborator

Can't say I like the number of adapters we'll have to add, so I guess my real question is whether using the pattern-matching switch from JDK21 is not enough to optimize the instanceof checks.

@rob-bygrave
Copy link
Contributor

the number of adapters we'll have to add

Yup. Far from ideal.

whether using the pattern-matching switch from JDK21 is not enough to optimize the instanceof checks

Or base a switch on the _type String, I wonder what that would look like ...

@rob-bygrave
Copy link
Contributor

More like:

    MaxKnownTypes(AdapterCreateRequest request) {
      super(request);
      this.targetType = request.targetType();
      final var attributes = request.attributes();
      this.value = (long) attributes.get("value");
    }

    @Override
    public boolean isValid(Number number) {
      // null values are valid
      if (number == null) {
        return true;
      }
      return switch (targetType) {
        case "Integer", "Long", "Short", "Byte" -> number.longValue() <= value;
        case "Double" -> compareDouble(number.doubleValue(), value, GREATER_THAN)  <= 0;
        case "Float" -> compareFloat((Float)number, value, GREATER_THAN)  <= 0;
        default -> throw new IllegalStateException();
      };
    }

Copy link
Collaborator

@SentryMan SentryMan left a comment

Choose a reason for hiding this comment

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

still wanna do the pattern-matching switch, but I can work with this.

@rbygrave rbygrave merged commit 8a4cfd0 into main Aug 1, 2023
@rbygrave rbygrave deleted the feature_attribute_type branch August 1, 2023 09:05
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.

Pondering passing the actual value type when creating the validator ... (to optimise number comparison checks)
3 participants