HHH 14241 Support ImplicitNamingStrategyComponentPathImpl with IdClass

[jrenaat]

No description provided.

[dreab8]

It looks good to me 👍

[sebersole]

Generally I am not a fan of processing Strings that are the result of other String processing.

E.g. here you allow a recursive call to continue building a String(Builder/Buffer). You then return that buffer in full and perform a sub-string on it. I think a better solution would be to handle this within the #process method instead

[dreab8]

@sebersole in this particular case is it not better in terms ofr performance to try the replacement just once in the transformAttributePath method, instead of multiple times inside the process method?

[sebersole]

@dreab8 Fair point. But unless I am mistaken, these only ever appear at the start of a path. So in #process you would already be limited to the attributePath.getParent() == null case, which we already check for anyway:

	public static void process(AttributePath attributePath, StringBuilder sb) {
		if ( attributePath.getParent() != null ) {
			process( attributePath.getParent(), sb );
			if ( StringHelper.isNotEmpty( attributePath.getParent().getProperty() ) ) {
				sb.append( '_' );
			}
		}
		else {
			// I added this else - this where we would check attributePath.getProperty() for "_identifierMapper"
		}

		String property = attributePath.getProperty();
		property = property.replace( "<", "" );
		property = property.replace( ">", "" );

		sb.append( property );
	}

In fact, I'd change that like:

	public static boolean process(AttributePath attributePath, StringBuilder sb) {
		String property = attributePath.getProperty();

		if ( attributePath.getParent() != null ) {
			final parentAdded = process( attributePath.getParent(), sb );
			if ( parentAdded ) ) {
				sb.append( '_' );
			}
		}
		else if ( "_identifierMapper".equals( property ) {
			// skip it, do not pass go
			return false;
		}

		property = property.replace( "<", "" );
		property = property.replace( ">", "" );

		sb.append( property );

		return true;
	}

I think that will be more efficient, but cannot say for sure I guess

[sebersole]

Either way, that's just a matter of timing. Good job finding the problem and solution @jrenaat

[sebersole]

I'm going to remove the request for changes. As Andrea points out, the performance difference is probably not worth requiring a change

[jrenaat]

Good job finding the problem and solution @jrenaat

Ok, some irony here @sebersole ... ;DD
But I can certainly appreciate that, thanks for the help

[dreab8]

@sebersole I think your idea of checking "_identifierMapper".equals( property ) inside the process method is the best in term of performance. I just noticed that attributePath.getParent() can be not null but with a property with an empty string value, so probably the change should be

public static void process(AttributePath attributePath, StringBuilder sb) {
		String property = attributePath.getProperty();

		final AttributePath parent = attributePath.getParent();
		if ( parent != null && !parent.getProperty().equals( "" ) ) {
			process( parent, sb );
			if ( StringHelper.isNotEmpty( parent.getProperty() ) ) {
				sb.append( '_' );
			}
		}
		else if ( PropertyPath.IDENTIFIER_MAPPER_PROPERTY.equals( property ) ) {
			// skip it, do not pass go
			sb.append( "id" );
			return;
		}
		property = property.replace( "<", "" );
		property = property.replace( ">", "" );

		sb.append( property );
	}

@jrenaat really good job 👍

[sebersole]

@dreab8 You can remove the following block now too:

if ( StringHelper.isNotEmpty( parent.getProperty() ) ) {
	sb.append( '_' );
}

public static void process(AttributePath attributePath, StringBuilder sb) {
		String property = attributePath.getProperty();

		final AttributePath parent = attributePath.getParent();
		if ( parent != null && StringHelper.isNotEmpty( parent.getProperty() ) ) {
			process( parent, sb );
			sb.append( '_' );
		}
		...
}

[NathanQingyangXu]

@jrenaat you enjoy the luxury to get @sebersole 's attention. My others would envy you a lot (including me), :).

[sebersole]

@jrenaat you enjoy the luxury to get @sebersole 's attention. My others would envy you a lot (including me), :).

Sorry Nathan, I have been taking a little break from 6.0 dev for the past 2 weeks or so to work on some Gradle and Quarkus stuff. I'll get back to your commits :)

[jrenaat]

@jrenaat you enjoy the luxury to get @sebersole 's attention. My others would envy you a lot (including me), :).

I'm blackmailing him ;)
Anyway, thanks for your comments, I've taken most of them into account