HHH-15725 Criteria API Expression.as adds cast even when the cast type is equal to the expression type

[dreab8]

https://hibernate.atlassian.net/browse/HHH-15725
This is an alternative solution to #5634

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

[beikov]

Suggested change

	if ( getExpressible() != null ) {
	final BasicType<X> basicTypeForJavaType = nodeBuilder().getTypeConfiguration()
	.getBasicTypeForJavaType( type );
	if ( basicTypeForJavaType != null
	&& getExpressible().getRelationalJavaType().getJavaTypeClass()
	== basicTypeForJavaType.getRelationalJavaType().getJavaTypeClass() ) {
	return new AsWrapperSqmExpression<>( this, type );
	}
	}
	final SqmExpressible<?> expressible = getExpressible();
	if ( expressible != null ) {
	if ( expressible.getExpressibleJavaType().getJavaTypeClass() == type ) {
	return this;
	}
	final BasicType<X> basicTypeForJavaType = nodeBuilder().getTypeConfiguration()
	.getBasicTypeForJavaType( type );
	if ( basicTypeForJavaType == null ) {
	throw new IllegalArgumentException( "Can't cast expression to unknown type: " + type.getCanonicalName() );
	}
	if ( expressible.getRelationalJavaType().getJavaTypeClass()
	== basicTypeForJavaType.getRelationalJavaType().getJavaTypeClass() ) {
	return new AsWrapperSqmExpression<>( basicTypeForJavaType, this );
	}
	}

[beikov]

Suggested change

	public AsWrapperSqmExpression(SqmExpression<?> expression, Class<T> type) {
	super(
	expression.nodeBuilder().getTypeConfiguration().getBasicTypeForJavaType( type ),
	expression.nodeBuilder()
	);
	this.expression = expression;
	}
	AsWrapperSqmExpression(SqmExpressible<T> type, SqmExpression<?> expression) {
	public AsWrapperSqmExpression(SqmExpressible<T> type, SqmExpression<?> expression) {

[beikov]

We have to decide on a HQL representation for this expression. I propose wrap(.. as ..)

Suggested change

	expression.appendHqlString( sb );
	}
	sb.append( "wrap(" );
	expression.appendHqlString( sb );
	sb.append( " as " );
	sb.append( getNodeType().getReturnedClassName() );
	sb.append( ")" );
	}
	@Override
	public <X> SqmExpression<X> as(Class<X> type) {
	return expression.as( type );
	}

[gavinking]

The Javadoc for Expression.as() states:

Perform a typecast upon the expression, returning a new expression object. Unlike cast(Class), this method does not cause type conversion: the runtime type is not changed.

I believe that the current implementation:

	@Override
	public <X> SqmExpression<X> as(Class<X> type) {
		return nodeBuilder().cast( this, type );
	}

is just completely wrong. Calling as() should never result in a cast().

[gavinking]

The correct implementation is probably something like:

	@Override @SuppressWarnings("unchecked")
	public <X> SqmExpression<X> as(Class<X> type) {
		return (SqmExpression<X>) this;
	}

[beikov]

returning a new expression object

Thanks for pointing that out. I guess we should always return a new AsWrapperSqmExpression then to comply with the spec? Though that brings up the question of backwards compatibility... Casting was done ever since ORM 3.6. Also see eb0396b#diff-44712025cd21f3fd778471c03a92e74170e252025ef123f56fc00b7d0c8d362bR45

[beikov]

Side note, EclipseLink always returns the same instance again and just does an unsafe type cast, which kind of also goes against the new part of that specification. I guess the spec should be clarified. Also, the TCK does not have a test for the as() method, so it's hard to say for sure what the intention was.

[gavinking]

@beikov Yeah, I agree, that's pretty weird, especially since IIRC Mike wrote that Javadoc.

OTOH, "returning a new expression object" is pretty explicit to me—I would say no clarification is needed.

[dreab8]

superseded by #8681