Skip to content
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

DBUTILS-150: Provide support for IndexedPropertyDescriptor #180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cstmgl
Copy link

@cstmgl cstmgl commented Feb 13, 2023

Provide support for IndexedPropertyDescriptor

@cstmgl
Copy link
Author

cstmgl commented Feb 13, 2023

maybe, did not test, this could also be fixed by changing the GenerousBeanProcessor to ensure that props[i].getPropertyType() is not null?

https://github.com/apache/commons-dbutils/blob/master/src/main/java/org/apache/commons/dbutils/GenerousBeanProcessor.java

      for (int i = 0; i < props.length; i++) {
        // Skip IndexedPropertyDescriptor
        if (props[i] instanceof IndexedPropertyDescriptor) {
          continue;
        }

Ok I tested this solution and did not managed to make it work, I believe I really need to consider also the IndexedPropertyDescriptor

@thecarlhall
Copy link
Contributor

Hi, @cstmgl ! Would you please tell me more about what you're seeing that brought you to this change? Could you share any exceptions, error messages, or oddities you've seen? On first scan of the code and the API doc you shared, I don't understand what effect this PR has. I'm keying particularly on this statement from the javadoc,

If a property is indexed, then its entry in the result array belongs to the IndexedPropertyDescriptor subclass of the PropertyDescriptor class

From this and the submitted code change, the caller outside of this method would still need to cast the property descriptor to IndexedPropertyDescriptor. This leaves me confused to what the suggested change does downstream.

@cstmgl
Copy link
Author

cstmgl commented Feb 14, 2023

Hi, @cstmgl ! Would you please tell me more about what you're seeing that brought you to this change? Could you share any exceptions, error messages, or oddities you've seen? On first scan of the code and the API doc you shared, I don't understand what effect this PR has. I'm keying particularly on this statement from the javadoc,

If a property is indexed, then its entry in the result array belongs to the IndexedPropertyDescriptor subclass of the PropertyDescriptor class

From this and the submitted code change, the caller outside of this method would still need to cast the property descriptor to IndexedPropertyDescriptor. This leaves me confused to what the suggested change does downstream.

I'll try to explain the best I can and sorry if the details are confusing, I wanted to share also some debug print screens but I can't seem to be able to do it.
If anything I'll try to update the ticket.

So the experience I found was:

  • I have a couple of LOBs (BLOBs) in the database and when I try to load them to my Bean those fields were always null (not all but the ones that are IndexedPropertyDescriptor
  • I could not understand why initially but then I noticed that it was because the result was never fetch because "processColumn" was never called
  • with some debugs I found out that the propType was actually null so the process column was skipped
      if (propType != null) {
        value = this.processColumn(rs, i, propType);

        if (value == null && propType.isPrimitive()) {
          value = primitiveDefaults.get(propType);
        }
      }
  • the reason for why it would be null was a bit un-expected but it really seems that while IndexedPropertyDescriptor is an extension of PropertyDescriptor by some reason the getPropertyType() always returns null and it expects to be called getIndexedPropertyType() I don't know why

getPropertyTyp is null
getIndexedPropertyType is not null

@thecarlhall
Copy link
Contributor

Thank you so much for the helpful description and screenshots! I believe I understand the problem now, and I think I have enough to write some tests around this.
IndexedPropertyType also has getIndexedReadMethod() and getIndexedWriteMethod(), so I want to understand the implications of those as well. Our implementation looks fairly safe from this, but I'd like to be sure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants