Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 314172 - StackOverflowError in ElementCollection validation
Summary: StackOverflowError in ElementCollection validation
Status: VERIFIED FIXED
Alias: None
Product: Dali JPA Tools
Classification: WebTools
Component: General (show other bugs)
Version: 2.3   Edit
Hardware: PC Windows Vista
: P3 major (vote)
Target Milestone: 2.3 RC3   Edit
Assignee: Paul Fullbright CLA
QA Contact:
URL:
Whiteboard: PMC_approved
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-24 18:28 EDT by Paul Fullbright CLA
Modified: 2010-06-01 15:38 EDT (History)
4 users (show)

See Also:
david_williams: pmc_approved+
neil.hauge: pmc_approved? (raghunathan.srinivasan)
neil.hauge: pmc_approved? (naci.dai)
deboer: pmc_approved+
neil.hauge: pmc_approved? (neil.hauge)
neil.hauge: pmc_approved? (kaloyan)
neil.hauge: review+


Attachments
stack trace (2.29 KB, text/plain)
2010-05-24 18:28 EDT, Paul Fullbright CLA
no flags Details
patch for karen (1.32 KB, patch)
2010-05-25 14:57 EDT, Paul Fullbright CLA
no flags Details | Diff
candidate patch (5.89 KB, patch)
2010-05-25 18:42 EDT, Paul Fullbright CLA
no flags Details | Diff
candidate patch B (1017 bytes, patch)
2010-05-25 18:50 EDT, Paul Fullbright CLA
no flags Details | Diff
candidate patch C (2.83 KB, patch)
2010-05-26 13:18 EDT, Karen Butzke CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Paul Fullbright CLA 2010-05-24 18:28:16 EDT
Created attachment 169744 [details]
stack trace

Given the following example:

@Embeddable
public class EmbeddableType {
	private String basic;
}


@Entity
@Access(PROPERTY)
public class EntityType extends MappedSuperclassType {
	
	private List<EmbeddableType> elementCollection;
	
	@ElementCollection
	protected List<EmbeddableType> getElementCollection() {
		return this.elementCollection;
	}
	
	protected void setElementCollection(List<EmbeddableType> elementCollection) {
		this.elementCollection = elementCollection;
	}
}


<?xml version="1.0" encoding="UTF-8"?>
<entity-mappings version="2.0" xmlns="http://java.sun.com/xml/ns/persistence/orm" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://java.sun.com/xml/ns/persistence/orm http://java.sun.com/xml/ns/persistence/orm_2_0.xsd">
	
	<entity class="foo.EntityType">
		
	</entity>
	
</entity-mappings>


Generate tables on a database and validate.  The attached stack trace occurs.

I don't think this is a simple validation error.  In this example, there are supposedly specified attribute overrides in the xml, which is clearly not the case.  I think this is an update error first.
Comment 1 Neil Hauge CLA 2010-05-25 10:08:17 EDT
Investigate for RC3.
Comment 2 Paul Fullbright CLA 2010-05-25 14:57:02 EDT
Created attachment 169881 [details]
patch for karen

attaching a patch that fixes the error, but causes a test failure
Comment 3 Paul Fullbright CLA 2010-05-25 18:42:43 EDT
Created attachment 169912 [details]
candidate patch

The previous patch does not work, as it ignores assumptions made about what is and what isn't a "specified" override on a virtual mapping.

This patch does add some API, so it might not be considered for RC3.

o.e.jpt.core.context.orm.OrmOverrideContainer
  added:
    Owner getOwner();

I'm not sure why this method wasn't there already, as the Owner interface is declared concurrent with the OrmOverrideContainer interface, and all known implementations implement that method (on a protected level).  At any rate, it is only a problem for extenders, since it is added API, and won't affect clients.

The bug fix attached simply redirects the attribute override text range to get the container's owner's text range rather than the container's text range.  A container never really has a text range at any rate - it almost always defers to its owner.
Comment 4 Paul Fullbright CLA 2010-05-25 18:50:27 EDT
Created attachment 169913 [details]
candidate patch B

Another option:  this patch duplicates the behavior of GenericOrmAssociationOverrideContainer.getValidationTextRange() onto GenericOrmAttributeOverrideContainer.  It removes some small functionality by pushing the text range up a level when there are existing attribute overrides, but it fixes the issue without changing API.
Comment 5 Karen Butzke CLA 2010-05-26 13:18:30 EDT
Created attachment 170055 [details]
candidate patch C

Paul, this patch is an extension of candidate patch B that fixes the one place that depended on the code you removed for getting the validation error actually on a specified attribute override. I haven't had a chance to test this yet, but will soon.  Let me know what you think.  patch B and patch C aren't necessarily the best solution going forward, but they are low risk and isolated for RC3.
Comment 6 Neil Hauge CLA 2010-05-26 16:53:10 EDT
* Explain why you believe this is a stop-ship defect. Or, if it is a "hotbug" (requested by an adopter) please document it as such. 

Stack overflow shuts down validation thread, rendering that functionality useless.  The error may also result in workspace instability.

* Is there a work-around? If so, why do you believe the work-around is insufficient? 

Only workaround is to avoid the use case that leads to this error.

* How has the fix been tested? Is there a test case attached to the bugzilla record? Has a JUnit Test been added? 

The patch (C) has been manually tested by Paul and Karen.  

* Give a brief technical overview. Who has reviewed this fix? 

See comment 4 and comment 5.  Patch C addresses a small regression in functionality that occurs with patch B.  This change just ensures that where we can correctly place validation in this scenario, we do, avoiding the problematic case.  Paul and I have reviewed the fix.

* What is the risk associated with this fix? 

Low risk, isolated for for RC3.
Comment 7 Neil Hauge CLA 2010-05-26 21:18:01 EDT
I should add that both Paul and Karen tested patch C and found no issues.  They didn't add a comment to the bug so I wanted to make sure that it was noted.
Comment 8 Paul Fullbright CLA 2010-05-27 14:16:08 EDT
Committed patch C for RC3 respin
Comment 9 Paul Fullbright CLA 2010-06-01 15:38:21 EDT
Verified in RC3