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

different default value between XMLStatementBuilder and MapperAnnotationBuilder #1334

Closed
bringyou opened this Issue Aug 24, 2018 · 4 comments

Comments

Projects
None yet
4 participants
@bringyou
Copy link

bringyou commented Aug 24, 2018

Hi, I found some differences between XmlStatementBuilder and MapperAnnotationBuilder when they parse SQL statement, they will give some variables different values when user didn't set. Sometimes, it can cause different behaviors.
For example, here is some code from XMLStatementBuilder

void parseStatementNode() {
    // other codes are omitted
    String resultSetType = context.getStringAttribute("resultSetType");
    ResultSetType resultSetTypeEnum = resolveResultSetType(resultSetType);
    // omit some codes
    builderAssistant.addMappedStatement(id, sqlSource, statementType, sqlCommandType,
        fetchSize, timeout, parameterMap, parameterTypeClass, resultMap, resultTypeClass,
        resultSetTypeEnum, flushCache, useCache, resultOrdered, 
        keyGenerator, keyProperty, keyColumn, databaseId, langDriver, resultSets);
}   
// inherited from BaseBuilder
protected ResultSetType resolveResultSetType(String alias) {
    if (alias == null) {
      return null;
    }
    try {
      return ResultSetType.valueOf(alias);
    } catch (IllegalArgumentException e) {
      throw new BuilderException("Error resolving ResultSetType. Cause: " + e, e);
    }
  }

Here is some code from MapperAnnotationBuilder

 void parseStatement(Method method) {
// omits unrelated codes
   Options options = method.getAnnotation(Options.class);
   ResultSetType resultSetType = ResultSetType.FORWARD_ONLY;
   if (options != null) {
        resultSetType = options.resultSetType();
      }
}

So we can see, users get different resultSetType value when they didn't set, one is null and the other is ResultSetType.FORWARD_ONLY. This may cause some different behaviors when others' code depend on the value.

MyBatis version

3.4.6

@rongbo-j

This comment has been minimized.

Copy link

rongbo-j commented Aug 29, 2018

the default ResultSetType depend on jdbc driver , most jdbc driver default ResultSetType is FORWARD_ONLY if not assigned . it's better to keep XMLStatementBuilder and MapperAnnotationBuilder has same default ResultSetType ;

@harawata

This comment has been minimized.

Copy link
Member

harawata commented Aug 29, 2018

Thank you @bringyou for the report and @rongbo-j for the comment.

The doc says "Default is unset (driver dependent).", so we may need to fix the annotation builder.

@harawata harawata added the bug label Aug 29, 2018

kazuki43zoo added a commit to kazuki43zoo/mybatis-3 that referenced this issue Sep 5, 2018

kazuki43zoo added a commit to kazuki43zoo/mybatis-3 that referenced this issue Sep 5, 2018

kazuki43zoo added a commit to kazuki43zoo/mybatis-3 that referenced this issue Sep 6, 2018

@kazuki43zoo kazuki43zoo self-assigned this Sep 7, 2018

@kazuki43zoo kazuki43zoo added this to the 3.5.0 milestone Sep 7, 2018

@kazuki43zoo

This comment has been minimized.

Copy link
Member

kazuki43zoo commented Sep 7, 2018

About backward compatibility

XML based mapper

Keep backward compatibility.

Annotation based mapper

Fetching data is same. (= Keep backward compatibility)
However, If the resultSetType attribute is omitted using annotation driven mapper, internal processing of pagination may change. In version 3.4 or before, it apply FORWARD_ONLY(=move to offset position by loop). But since 3.5, it depends on JDBC driver default behavior.

@bringyou

This comment has been minimized.

Copy link
Author

bringyou commented Sep 9, 2018

thank you guys

kazuki43zoo added a commit to kazuki43zoo/mybatis-3 that referenced this issue Dec 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment