[GitHub] [openjpa] eolivelli commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660336943


   try again:
   https://github.com/eolivelli/openjpa/runs/883449807



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660336352


   It looks like the ant plugin looks for openjpa lib into local Maven 
repository.
   I will change the goal to 'install' as @ilgrosso suggested



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660335970


   now I see why you asked for 'install'
   the ant plugin needs the jars into the maven repository



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli removed a comment on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli removed a comment on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660335970


   now I see why you asked for 'install'
   the ant plugin needs the jars into the maven repository



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660335524


   all tests passed locally



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli edited a comment on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli edited a comment on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660335260


   this is a demo run:
   https://github.com/eolivelli/openjpa/runs/883352848
   
   fingers crossed :-) 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660335260


   this is a demo run:
   https://github.com/eolivelli/openjpa/runs/883352848



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660320903


   The build fails with verify
   
   [ERROR] 
/home/runner/work/openjpa/openjpa/openjpa-examples/openbooks/build.xml:117: *** 
Error:
   [ERROR] The directory for OpenJPA libraries can not be 
located at 
/home/runner/.m2/repository/org/apache/openjpa/openjpa-all/3.1.3-SNAPSHOT.
   [ERROR] Make sure openjpa.lib property value is correct in 
build.properties file.
   [ERROR] around Ant part ...

[GitHub] [openjpa] eolivelli commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660279740


   If you commit this change github actions will automatically be activated 
probably 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660279543


   @ilgrosso done



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] ilgrosso commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

ilgrosso commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660275467


   Agree then, go on with verify



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660260140


   @ilgrosso thanks for your review
   
   There is not need to 'install'.
   With 'install' Maven copies the jar to .m2.
   
   It looks like the build works without it.
   Probably 'verify' could work ad well 



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] ilgrosso commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

ilgrosso commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660246008


   @eolivelli why "only" `package`? Why not `install`?
   
   @rmannibucau +1 to add it to push as well.



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] rmannibucau commented on pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

rmannibucau commented on pull request #70:
URL: https://github.com/apache/openjpa/pull/70#issuecomment-660207978


   Looks good to me now, only thing to decide is if we keep on push or not 
(does not hurt I think and can be good for us).
   Waiting for a passing build then I think we can merge.
   
   
   cc @ilgrosso @struberg want to have a look?



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli opened a new pull request #70: OPENJPA-2819 Add simple GitHub Actions validation for Pull Requests

[GitBox]

eolivelli opened a new pull request #70:
URL: https://github.com/apache/openjpa/pull/70


   



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456520863



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -312,11 +315,20 @@ public String getResourceName() {
 return _rels;
 }
 
+/**
+ * Return the list of column names, used only for informative (error) 
messages.
+ * @return
+ */
 public String[] getColumnNames() {
 if (_colMap == null) {
 return new String[0];
 }
-DBIdentifier[] sNames = _colMap.keySet().toArray(new 
DBIdentifier[_colMap.size()]);

Review comment:
   Hmm, maybe diff is not up to date but you still allocate 2 arrays, first 
one is a DBIdentifier array instead of an ArrayList but it is ~the same size 
(I'm thinking to the case we have > 200 columns in the db and it happens) so I 
would just do:
   
   _colMap
   .values()
   .stream()
   .map(Column::getIdentifier)
   .map(DBIdentifier::getName)
   .toArray(String[]::new);





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456519781



##
File path: 
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/SelectImpl.java
##
@@ -2711,7 +2711,7 @@ private String getColumnAlias(Column col, PathJoins pj) {
 return alias + "_" + col;
 }
 alias = SelectImpl.toAlias(_sel.getTableIndex(col.getTable(), pj, 
false));
-return (alias == null) ? null : alias + "." + col;
+return (alias == null) ? null : alias + "." + 
_sel._dict.getNamingUtil().toDBName(col.toString());

Review comment:
   you can likely create an assert on the select string directly - even 
using internals to instantiate it if it helps.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456519302



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
 return getColumn(DBIdentifier.newIdentifier(name, 
DBIdentifierType.COLUMN, true));
 }
 
-public Column getColumn(DBIdentifier name) {
+private Column internalGetColumn(DBIdentifier name) {
 if (DBIdentifier.isNull(name) || _colMap == null)
 return null;
-return _colMap.get(DBIdentifier.toUpper(name));
+DBIdentifier key = normalizeColumnKey(name);
+return _colMap.get(key);
+}
+
+public Column getColumn(DBIdentifier name) {
+return internalGetColumn(name);
+}
+
+private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));

Review comment:
   It is not really about the overhead (should be a "contains" in terms of 
complexity for most cases) but more about being fully deterministic and not 
"random".
   
   Why is it hard?
   Tables can be created in:
   - 
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/DBDictionary.java#newTable
   - 
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/DBDictionary.java#getValidColumnName
   - org.apache.openjpa.jdbc.schema.Schema#addTable
   - org.apache.openjpa.jdbc.schema.SchemaGroup#newTable
   
   Now if you take 1 more level you always have the dictionnary handy so if 
simpler we can just add the dict in the table.
   
   Alternative I had in mind was just to handle it in 
org.apache.openjpa.jdbc.sql.DBDictionary#getValidColumnName(org.apache.openjpa.jdbc.identifier.DBIdentifier,
 org.apache.openjpa.jdbc.schema.Table, boolean) in herddb dict and set the flag 
there.
   It is not about tracking it is delimited or not but tracking that the dict 
is in delimited mode or not, then you would do 
if identifier.delimited then removeDelimited() else noop.
   





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456514057



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -312,11 +315,20 @@ public String getResourceName() {
 return _rels;
 }
 
+/**
+ * Return the list of column names, used only for informative (error) 
messages.
+ * @return
+ */
 public String[] getColumnNames() {
 if (_colMap == null) {
 return new String[0];
 }
-DBIdentifier[] sNames = _colMap.keySet().toArray(new 
DBIdentifier[_colMap.size()]);

Review comment:
   yes I understood and I have updated the code, thanks





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456512187



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -312,11 +315,20 @@ public String getResourceName() {
 return _rels;
 }
 
+/**
+ * Return the list of column names, used only for informative (error) 
messages.
+ * @return
+ */
 public String[] getColumnNames() {
 if (_colMap == null) {
 return new String[0];
 }
-DBIdentifier[] sNames = _colMap.keySet().toArray(new 
DBIdentifier[_colMap.size()]);

Review comment:
   Proposal was just to not to toList().toArray which allocates twice for 
no real reason the same structure. Probably missed a .map in my snippet but the 
double allocation should be avoided IMHO.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456500957



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
 return getColumn(DBIdentifier.newIdentifier(name, 
DBIdentifierType.COLUMN, true));
 }
 
-public Column getColumn(DBIdentifier name) {
+private Column internalGetColumn(DBIdentifier name) {
 if (DBIdentifier.isNull(name) || _colMap == null)
 return null;
-return _colMap.get(DBIdentifier.toUpper(name));
+DBIdentifier key = normalizeColumnKey(name);
+return _colMap.get(key);
+}
+
+public Column getColumn(DBIdentifier name) {
+return internalGetColumn(name);
+}
+
+private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));

Review comment:
   > Makes sense to me to know if the identifier had been quoted or not and 
does not require a single signature change in methods
   
   I would like this change but it is not trivial, as I see that sometimes we 
are manipulating the internal `_name` of DBIdentifier, and tracking when it is 
already delimited or not seems to me an hard task.
   It can be done in a follow up patch. I will be happy to follow up in this 
direction
   
   I am trying now to reduce the overhead of removeDelimiters, it does 
unnecessary memory allocations





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456502732



##
File path: 
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/sql/SelectImpl.java
##
@@ -2711,7 +2711,7 @@ private String getColumnAlias(Column col, PathJoins pj) {
 return alias + "_" + col;
 }
 alias = SelectImpl.toAlias(_sel.getTableIndex(col.getTable(), pj, 
false));
-return (alias == null) ? null : alias + "." + col;
+return (alias == null) ? null : alias + "." + 
_sel._dict.getNamingUtil().toDBName(col.toString());

Review comment:
   the new test exercise this line.
   I must be honest, with Derby it does not fail the test,  even without this 
line, but without this change the integration tests with HerdDB fail





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456501604



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -312,11 +315,20 @@ public String getResourceName() {
 return _rels;
 }
 
+/**
+ * Return the list of column names, used only for informative (error) 
messages.
+ * @return
+ */
 public String[] getColumnNames() {
 if (_colMap == null) {
 return new String[0];
 }
-DBIdentifier[] sNames = _colMap.keySet().toArray(new 
DBIdentifier[_colMap.size()]);

Review comment:
   done





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456500957



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
 return getColumn(DBIdentifier.newIdentifier(name, 
DBIdentifierType.COLUMN, true));
 }
 
-public Column getColumn(DBIdentifier name) {
+private Column internalGetColumn(DBIdentifier name) {
 if (DBIdentifier.isNull(name) || _colMap == null)
 return null;
-return _colMap.get(DBIdentifier.toUpper(name));
+DBIdentifier key = normalizeColumnKey(name);
+return _colMap.get(key);
+}
+
+public Column getColumn(DBIdentifier name) {
+return internalGetColumn(name);
+}
+
+private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));

Review comment:
   > Makes sense to me to know if the identifier had been quoted or not and 
does not require a single signature change in methods
   I would like this change but it is not trivial, as I see that sometimes we 
are manipulating the internal `_name` of DBIdentifier, and tracking when it is 
already delimited or not seems to me an hard task.
   It can be done in a follow up patch. I will be happy to follow up in this 
direction
   





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456499123



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -312,11 +315,20 @@ public String getResourceName() {
 return _rels;
 }
 
+/**
+ * Return the list of column names, used only for informative (error) 
messages.
+ * @return
+ */
 public String[] getColumnNames() {
 if (_colMap == null) {
 return new String[0];
 }
-DBIdentifier[] sNames = _colMap.keySet().toArray(new 
DBIdentifier[_colMap.size()]);

Review comment:
   @rmannibucau 
   I can't get your suggestion
   If I change this to:
   ```
   _colMap
   .values()
   .stream()
   .map(Column::getIdentifier)
   .toArray(DBIdentifier[]::new);
   ```
   I get a DBIdentifier[] and not a String[]
   
   





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456494234



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
 return getColumn(DBIdentifier.newIdentifier(name, 
DBIdentifierType.COLUMN, true));
 }
 
-public Column getColumn(DBIdentifier name) {
+private Column internalGetColumn(DBIdentifier name) {
 if (DBIdentifier.isNull(name) || _colMap == null)
 return null;
-return _colMap.get(DBIdentifier.toUpper(name));
+DBIdentifier key = normalizeColumnKey(name);
+return _colMap.get(key);
+}
+
+public Column getColumn(DBIdentifier name) {
+return internalGetColumn(name);
+}
+
+private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));

Review comment:
   the identifier should always (i know a few cases where it is not the 
case but shouldnt impact you or means it must be fixed) be created from the 
dict so just inject that state in the identifier? Makes sense to me to know if 
the identifier had been quoted or not and does not require a single signature 
change in methods.





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [openjpa] rmannibucau commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

rmannibucau commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456493228



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -312,11 +315,20 @@ public String getResourceName() {
 return _rels;
 }
 
+/**
+ * Return the list of column names, used only for informative (error) 
messages.
+ * @return
+ */
 public String[] getColumnNames() {
 if (_colMap == null) {
 return new String[0];
 }
-DBIdentifier[] sNames = _colMap.keySet().toArray(new 
DBIdentifier[_colMap.size()]);

Review comment:
   _colMap
   .values()
   .stream()
   .map(Column::getIdentifier)
   .toArray(DBIdentifier[]::new);





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: Adding GitHub Actions for PR validation

[Romain Manni-Bucau]

Jenkins builds are used AFAIK (but likely most of them by people doing
$upport on older versions ;)) Personally I only use master but if you want
to start a cleanup thread +1.

It also seems unlikely we get PR support on jenkins from my understanding -
if wrong it would be awesome - this is why I think we should enable PR
somehow now.

Now just a small note on travis vs gh actions and why I don't defend
travis*.org* much: gh actions - gh link is highly unlikely to break with gh
actions (compared to the link with travis which can and do) and travis.org
is no more maintained since years (can break any day and the best the
support will do is to ask you to move to travis.com, often for free for OSS
but still), so it is likely a best bet *today* to support gh actions than
it was last time to support travis IMHO (yes suffered a bit from
travis*.org* last years).

Hope it makes sense.

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book



Le ven. 17 juil. 2020 à 16:31, Francesco Chicchiriccò 
a écrit :

> On 17/07/20 16:19, Enrico Olivelli wrote:
> > Il Ven 17 Lug 2020, 16:14 Romain Manni-Bucau  ha
> > scritto:
> >
> >> Hi Enrico,
> >>
> >> While we have a single type of this build tool +1 (= I don't think it
> would
> >> be good to have GH Action + Travis + AppVeryor + SemaphoreCI + ...).
> >> What would do the default build? mvn install or would it run all DB
> >> profiles based on docker images?
> > Just mvn install is enough for me.
> >
> > More complex/heavier builds should stay on ASF Jenkins, no need to run
> them
> > at every PR validation.
>
> Hi,
> chiming in just to remember that we already had such discussion [1],  that
> we don't have Travis CI set up currently (I have it on my own fork, though:
> [2]) and that we have a whole set of (mostly useless?) builds set up at ASF
> Jenkins: [3].
>
> Anyway, +1 to GitHub actions for PR validation.
>
> Regards.
>
> [1]
> https://lists.apache.org/thread.html/82739677363b480e0913496ccd33e449625052e821f4b0275f6101c6%40%3Cdev.openjpa.apache.org%3E
> [2] https://github.com/ilgrosso/openjpa/blob/master/.travis.yml
> [3] https://builds.apache.org/view/M-R/view/OpenJPA/
>
> >> Romain Manni-Bucau
> >> @rmannibucau  |  Blog
> >>  | Old Blog
> >>  | Github <
> >> https://github.com/rmannibucau> |
> >> LinkedIn  | Book
> >> <
> >>
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >>
> >> Le ven. 17 juil. 2020 à 15:54, Enrico Olivelli  a
> >> écrit :
> >>
> >>> Hey,
> >>> what about adding a simple validation of Pull Requests using GitHub
> >> actions
> >>> ?
> >>> I can create a very simple configuration file if you like this idea.
> >>>
> >>> It will help a lot in contributing patches, this way it is sure for the
> >>> committer that the patch is not breaking unit tests (not integration
> >> tests,
> >>> of course) and it can help the contributor as it given an immediate
> >>> feedback of the goodness of the patch
> >>>
> >>>
> >>> Enrico
> >>>
>
> --
> Francesco Chicchiriccò
>
> Tirasa - Open Source Excellence
> http://www.tirasa.net/
>
> Member at The Apache Software Foundation
> Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
> http://home.apache.org/~ilgrosso/
>
>

Re: Adding GitHub Actions for PR validation

[Enrico Olivelli]

Here it is the ticket
https://issues.apache.org/jira/browse/OPENJPA-2819

I am going to send a patch
Enrico

Il giorno ven 17 lug 2020 alle ore 16:31 Francesco Chicchiriccò <
ilgro...@apache.org> ha scritto:

> On 17/07/20 16:19, Enrico Olivelli wrote:
> > Il Ven 17 Lug 2020, 16:14 Romain Manni-Bucau  ha
> > scritto:
> >
> >> Hi Enrico,
> >>
> >> While we have a single type of this build tool +1 (= I don't think it
> would
> >> be good to have GH Action + Travis + AppVeryor + SemaphoreCI + ...).
> >> What would do the default build? mvn install or would it run all DB
> >> profiles based on docker images?
> > Just mvn install is enough for me.
> >
> > More complex/heavier builds should stay on ASF Jenkins, no need to run
> them
> > at every PR validation.
>
> Hi,
> chiming in just to remember that we already had such discussion [1],  that
> we don't have Travis CI set up currently (I have it on my own fork, though:
> [2]) and that we have a whole set of (mostly useless?) builds set up at ASF
> Jenkins: [3].
>
> Anyway, +1 to GitHub actions for PR validation.
>
> Regards.
>
> [1]
> https://lists.apache.org/thread.html/82739677363b480e0913496ccd33e449625052e821f4b0275f6101c6%40%3Cdev.openjpa.apache.org%3E
> [2] https://github.com/ilgrosso/openjpa/blob/master/.travis.yml
> [3] https://builds.apache.org/view/M-R/view/OpenJPA/
>
> >> Romain Manni-Bucau
> >> @rmannibucau  |  Blog
> >>  | Old Blog
> >>  | Github <
> >> https://github.com/rmannibucau> |
> >> LinkedIn  | Book
> >> <
> >>
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >>
> >> Le ven. 17 juil. 2020 à 15:54, Enrico Olivelli  a
> >> écrit :
> >>
> >>> Hey,
> >>> what about adding a simple validation of Pull Requests using GitHub
> >> actions
> >>> ?
> >>> I can create a very simple configuration file if you like this idea.
> >>>
> >>> It will help a lot in contributing patches, this way it is sure for the
> >>> committer that the patch is not breaking unit tests (not integration
> >> tests,
> >>> of course) and it can help the contributor as it given an immediate
> >>> feedback of the goodness of the patch
> >>>
> >>>
> >>> Enrico
> >>>
>
> --
> Francesco Chicchiriccò
>
> Tirasa - Open Source Excellence
> http://www.tirasa.net/
>
> Member at The Apache Software Foundation
> Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
> http://home.apache.org/~ilgrosso/
>
>

[jira] [Created] (OPENJPA-2819) Add simple GitHub Actions validation for Pull Requests

[Enrico Olivelli (Jira)]

Enrico Olivelli created OPENJPA-2819:


 Summary: Add simple GitHub Actions validation for Pull Requests
 Key: OPENJPA-2819
 URL: https://issues.apache.org/jira/browse/OPENJPA-2819
 Project: OpenJPA
  Issue Type: Task
  Components: build / infrastructure
Affects Versions: 3.1.1
Reporter: Enrico Olivelli
 Fix For: 3.1.3


As discussed on the ML having a validation of Pull Requests will ease the life 
of contributors and committers.

This task will introduce a simple "mvn install" on JDK8 on Linux on GitHub 
actions



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

[GitHub] [openjpa] eolivelli commented on a change in pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on a change in pull request #69:
URL: https://github.com/apache/openjpa/pull/69#discussion_r456480105



##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -329,10 +341,19 @@ public Column getColumn(String name) {
 return getColumn(DBIdentifier.newIdentifier(name, 
DBIdentifierType.COLUMN, true));
 }
 
-public Column getColumn(DBIdentifier name) {
+private Column internalGetColumn(DBIdentifier name) {
 if (DBIdentifier.isNull(name) || _colMap == null)
 return null;
-return _colMap.get(DBIdentifier.toUpper(name));
+DBIdentifier key = normalizeColumnKey(name);
+return _colMap.get(key);
+}
+
+public Column getColumn(DBIdentifier name) {
+return internalGetColumn(name);
+}
+
+private DBIdentifier normalizeColumnKey(DBIdentifier name) {
+return DBIdentifier.removeDelimiters(DBIdentifier.toUpper(name, true));

Review comment:
   @rmannibucau about applying removeDelimiters only when needed:
   my original fix was to handle this only in SchemaTool, but just adding the 
new test case showed that  the situation is more complex.
   Table class does not have a reference to the DBDictionary and we can't 
change the signature of all methods, it is kind of Public API that can be 
overridden by custom DBDictionary implementations
   
   Tests are also failing if you switch to 
`DBIdentifier.toUpper(DBIdentifier.removeDelimiters(name), true);`
   
   IMHO we could also probably clean up some of those methods in order to 
reduce useless allocation, but it can be a separate follow up patch.
   

##
File path: openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/Table.java
##
@@ -312,11 +315,20 @@ public String getResourceName() {
 return _rels;
 }
 
+/**
+ * Return the list of column names, used only for informative (error) 
messages.
+ * @return
+ */
 public String[] getColumnNames() {
 if (_colMap == null) {
 return new String[0];
 }
-DBIdentifier[] sNames = _colMap.keySet().toArray(new 
DBIdentifier[_colMap.size()]);

Review comment:
   `_colMap.keySet()` contains an internal representation of column names 
(all uppercased)
   this method is used only for error messages and IMHO it is better to have a 
consistent handling of this internal data structure
   

##
File path: 
openjpa-jdbc/src/main/java/org/apache/openjpa/jdbc/schema/SchemaTool.java
##
@@ -1030,10 +1032,7 @@ private void drop(SchemaGroup db, SchemaGroup repos, 
boolean considerDatabaseSta
 continue;
 
 if (dropColumn(cols[k])) {
-if (dbTable != null)

Review comment:
   dbTable is always not null here, we have a null check 4 lines above,
   I can revert if you want





This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: Adding GitHub Actions for PR validation

[Francesco Chicchiriccò]

On 17/07/20 16:19, Enrico Olivelli wrote:
> Il Ven 17 Lug 2020, 16:14 Romain Manni-Bucau  ha
> scritto:
>
>> Hi Enrico,
>>
>> While we have a single type of this build tool +1 (= I don't think it would
>> be good to have GH Action + Travis + AppVeryor + SemaphoreCI + ...).
>> What would do the default build? mvn install or would it run all DB
>> profiles based on docker images?
> Just mvn install is enough for me.
>
> More complex/heavier builds should stay on ASF Jenkins, no need to run them
> at every PR validation.

Hi,
chiming in just to remember that we already had such discussion [1],  that we 
don't have Travis CI set up currently (I have it on my own fork, though: [2]) 
and that we have a whole set of (mostly useless?) builds set up at ASF Jenkins: 
[3].

Anyway, +1 to GitHub actions for PR validation.

Regards.

[1] 
https://lists.apache.org/thread.html/82739677363b480e0913496ccd33e449625052e821f4b0275f6101c6%40%3Cdev.openjpa.apache.org%3E
[2] https://github.com/ilgrosso/openjpa/blob/master/.travis.yml
[3] https://builds.apache.org/view/M-R/view/OpenJPA/

>> Romain Manni-Bucau
>> @rmannibucau  |  Blog
>>  | Old Blog
>>  | Github <
>> https://github.com/rmannibucau> |
>> LinkedIn  | Book
>> <
>> https://www.packtpub.com/application-development/java-ee-8-high-performance
>>
>> Le ven. 17 juil. 2020 à 15:54, Enrico Olivelli  a
>> écrit :
>>
>>> Hey,
>>> what about adding a simple validation of Pull Requests using GitHub
>> actions
>>> ?
>>> I can create a very simple configuration file if you like this idea.
>>>
>>> It will help a lot in contributing patches, this way it is sure for the
>>> committer that the patch is not breaking unit tests (not integration
>> tests,
>>> of course) and it can help the contributor as it given an immediate
>>> feedback of the goodness of the patch
>>>
>>>
>>> Enrico
>>>

-- 
Francesco Chicchiriccò

Tirasa - Open Source Excellence
http://www.tirasa.net/

Member at The Apache Software Foundation
Syncope, Cocoon, Olingo, CXF, OpenJPA, PonyMail
http://home.apache.org/~ilgrosso/

Re: Adding GitHub Actions for PR validation

[Romain Manni-Bucau]

works for me

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book



Le ven. 17 juil. 2020 à 16:19, Enrico Olivelli  a
écrit :

> Il Ven 17 Lug 2020, 16:14 Romain Manni-Bucau  ha
> scritto:
>
> > Hi Enrico,
> >
> > While we have a single type of this build tool +1 (= I don't think it
> would
> > be good to have GH Action + Travis + AppVeryor + SemaphoreCI + ...).
> > What would do the default build? mvn install or would it run all DB
> > profiles based on docker images?
> >
>
> Just mvn install is enough for me.
>
> More complex/heavier builds should stay on ASF Jenkins, no need to run them
> at every PR validation.
>
> Enrico
>
>
> > Romain Manni-Bucau
> > @rmannibucau  |  Blog
> >  | Old Blog
> >  | Github <
> > https://github.com/rmannibucau> |
> > LinkedIn  | Book
> > <
> >
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> > >
> >
> >
> > Le ven. 17 juil. 2020 à 15:54, Enrico Olivelli  a
> > écrit :
> >
> > > Hey,
> > > what about adding a simple validation of Pull Requests using GitHub
> > actions
> > > ?
> > > I can create a very simple configuration file if you like this idea.
> > >
> > > It will help a lot in contributing patches, this way it is sure for the
> > > committer that the patch is not breaking unit tests (not integration
> > tests,
> > > of course) and it can help the contributor as it given an immediate
> > > feedback of the goodness of the patch
> > >
> > >
> > >
> > > Enrico
> > >
> >
>

Re: Adding GitHub Actions for PR validation

[Enrico Olivelli]

Il Ven 17 Lug 2020, 16:14 Romain Manni-Bucau  ha
scritto:

> Hi Enrico,
>
> While we have a single type of this build tool +1 (= I don't think it would
> be good to have GH Action + Travis + AppVeryor + SemaphoreCI + ...).
> What would do the default build? mvn install or would it run all DB
> profiles based on docker images?
>

Just mvn install is enough for me.

More complex/heavier builds should stay on ASF Jenkins, no need to run them
at every PR validation.

Enrico


> Romain Manni-Bucau
> @rmannibucau  |  Blog
>  | Old Blog
>  | Github <
> https://github.com/rmannibucau> |
> LinkedIn  | Book
> <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
>
>
> Le ven. 17 juil. 2020 à 15:54, Enrico Olivelli  a
> écrit :
>
> > Hey,
> > what about adding a simple validation of Pull Requests using GitHub
> actions
> > ?
> > I can create a very simple configuration file if you like this idea.
> >
> > It will help a lot in contributing patches, this way it is sure for the
> > committer that the patch is not breaking unit tests (not integration
> tests,
> > of course) and it can help the contributor as it given an immediate
> > feedback of the goodness of the patch
> >
> >
> >
> > Enrico
> >
>

Re: Adding GitHub Actions for PR validation

[Romain Manni-Bucau]

Hi Enrico,

While we have a single type of this build tool +1 (= I don't think it would
be good to have GH Action + Travis + AppVeryor + SemaphoreCI + ...).
What would do the default build? mvn install or would it run all DB
profiles based on docker images?

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book



Le ven. 17 juil. 2020 à 15:54, Enrico Olivelli  a
écrit :

> Hey,
> what about adding a simple validation of Pull Requests using GitHub actions
> ?
> I can create a very simple configuration file if you like this idea.
>
> It will help a lot in contributing patches, this way it is sure for the
> committer that the patch is not breaking unit tests (not integration tests,
> of course) and it can help the contributor as it given an immediate
> feedback of the goodness of the patch
>
>
>
> Enrico
>

[GitHub] [openjpa] rmannibucau commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

rmannibucau commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660127090


   @eolivelli it is ok to fix herddb dict there since it is fully related IMHO. 
The first part of the diff is suspicious (schematool diff seems unnecessary and 
getColumnNames change is unlikely too). Would also be neat to not drop 
delimiters if the dictionnary does not use them (normalizeColumnKey). If it 
helps I think we can add a flag in DBIdentifier to notify it is delimited or 
not and have both the delimited and not delimited values (raw name vs statement 
name).



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Adding GitHub Actions for PR validation

[Enrico Olivelli]

Hey,
what about adding a simple validation of Pull Requests using GitHub actions
?
I can create a very simple configuration file if you like this idea.

It will help a lot in contributing patches, this way it is sure for the
committer that the patch is not breaking unit tests (not integration tests,
of course) and it can help the contributor as it given an immediate
feedback of the goodness of the patch



Enrico

[GitHub] [openjpa] eolivelli commented on pull request #69: OPENJPA-2818 DBDictionary > delimitIdentifiers = true does not work

[GitBox]

eolivelli commented on pull request #69:
URL: https://github.com/apache/openjpa/pull/69#issuecomment-660118706


   @rmannibucau the patch became bigger as soon as I tried to add test cases.
   Still it is not complete, I have to add a test case of the  
SelectImpl#getColumnAlias case
   
   This patch also is fixing HerdDBDictionary, if you prefer I can separate 
that change in HerdDBDictionary ticket



This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: A few runtime enhancement/bytecode questions

[Romain Manni-Bucau]

Le ven. 17 juil. 2020 à 11:27, Mark Struberg  a
écrit :

> answers inside.
>
> LieGrue,
> strub
>
> > Am 17.07.2020 um 11:14 schrieb Romain Manni-Bucau  >:
> >
> > Le ven. 17 juil. 2020 à 10:45, Mark Struberg 
> a
> > écrit :
> >
> >> Both prepareUnenhancedClasses and loadAgent already run in
> >> createEntityManagerFactory.
> >>
> >
> > Not in general, depends the conf and by default i run in none of both
> with
> > a standard config for ex for EE.
>
> Let me rephrase this: both those methods will get called in
> createEntityManagerFactory already. If they actually perform some bytecode
> enhancement is up to configuration and whether the classes have been
> enhanced otherwise before ;)
>
>
> >
> >
> >> crateEntityManager does not trigger enhancement afaict.
> >>
> >
> > createEMF does not since the creation of openjpa is lazy so the first
> > createEM does (in general case one again).
>
> Nope, it really gets called already by createEntityManagerFactory. You can
> debug into this if you want.
>

Did and I can guarantee you it is lazy if your conf does not enforce it
somehow.


>
> LieGrue,
> strub
>
>
> >
> >>
> >> LieGrue,
> >> strub
> >>
> >>> Am 17.07.2020 um 10:11 schrieb Romain Manni-Bucau <
> rmannibu...@gmail.com
> >>> :
> >>>
> >>> Hi Mark,
> >>>
> >>> If it helps:
> >>>
> >>> 1. org.apache.openjpa.kernel.AbstractBrokerFactory#postCreationCallback
> >>> only triggers the prepareUnenhancedClasses if preload=true (never the
> >> case
> >>> right? ;))
> >>> 2. Normally classes are loaded in
> >>> there org.apache.openjpa.kernel.AbstractBrokerFactory#initializeBroker
> >>> *when creating an em, not an emf* so better to have loaded ran the
> >>> enhancement early (loadAgent)
> >>> 3. Not sure it is done but it can happen 2 has a custom MDR not
> returning
> >>> right the classes - seems code is ok with that. I don't know if it is a
> >>> case we want to handle, I don't think it is needed.
> >>> 4. There is another case you didn't list where the EMF can be there but
> >> no
> >>> EM and enhancement should happen ASAP: deserialization ;)
> >>> 5. Finally there is a subtle difference between SE and EE case where
> one
> >>> will trigger the agent and the other will just add the transformer in
> the
> >>> classloader through PersistenUnitInfo (and container should handle this
> >>> additional properly), I don't know if our agent should be compatible
> with
> >>> it for apploader (jvm one).
> >>>
> >>> Romain Manni-Bucau
> >>> @rmannibucau  |  Blog
> >>>  | Old Blog
> >>>  | Github <
> >> https://github.com/rmannibucau> |
> >>> LinkedIn  | Book
> >>> <
> >>
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >>>
> >>>
> >>>
> >>> Le ven. 17 juil. 2020 à 09:35, Mark Struberg  >
> >> a
> >>> écrit :
> >>>
>  hi folks!
> 
>  I'm right now cleaning up old code in our enhancer. Like removing
> java5
>  hacks, etc
>  This is also triggered by our recent discussion about _transform and
> >> other
>  pieces.
>  Slowly getting my head into the right space for it. This is way more
>  complex than I remembered ;)
> 
>  So here is my next puzzle:
> 
>  I've created a new example with 2 classes: Person and Employee extends
>  Person
>  Plus a persistence.xml with enlisting those 2 classes (makes a
>  difference!) and essentially
>  
>  
>  The reason I switched on RuntimeUnenhancedClasses is that this check
> is
>  performed when the BrokerFactory gets created.
>  And if the classes are not build-time enhanced, then it fails in
> 
> >>
> org.apache.openjpa.enhance.ManagedClassSubclasser#prepareUnenhancedClasses
>  if (conf.getRuntimeUnenhancedClassesConstant() !=
>  RuntimeUnenhancedClassesModes.SUPPORTED) { ..
>  And if you switch to supported, then it will create a new subclass and
>  replaces field access to the getters/setters. Sadly
>  java.lang.instrument.Instrumentation#retransform cannot be used to add
> >> new
>  fields or methods. Otherwise we could do a direct full replacement of
> >> this
>  class.
> 
>  Fun thing is that later on in
> 
> >>
> org.apache.openjpa.persistence.PersistenceProviderImpl#createEntityManagerFactory(java.lang.String,
>  java.lang.String, java.util.Map)
>  we call loadAgent(factory);
>  But what is it supposed to do? Which cases does it serve?
>  If the classes did not have been already enhanced at build time nor
> via
>  javaagent they will now be subclassed by ManagedClassSubclasser.
>  And if we do not set RuntimeUnenhancedClasses=supported then we will
> >> never
>  make it so far but blow up way earlier.
> 
>  Also: it is nice to have that agent attached. But why? All the entity
>  classe already have been loaded already during BrokerFactory 

Re: A few runtime enhancement/bytecode questions

[Mark Struberg]

answers inside.

LieGrue,
strub

> Am 17.07.2020 um 11:14 schrieb Romain Manni-Bucau :
> 
> Le ven. 17 juil. 2020 à 10:45, Mark Struberg  a
> écrit :
> 
>> Both prepareUnenhancedClasses and loadAgent already run in
>> createEntityManagerFactory.
>> 
> 
> Not in general, depends the conf and by default i run in none of both with
> a standard config for ex for EE.

Let me rephrase this: both those methods will get called in 
createEntityManagerFactory already. If they actually perform some bytecode 
enhancement is up to configuration and whether the classes have been enhanced 
otherwise before ;)


> 
> 
>> crateEntityManager does not trigger enhancement afaict.
>> 
> 
> createEMF does not since the creation of openjpa is lazy so the first
> createEM does (in general case one again).

Nope, it really gets called already by createEntityManagerFactory. You can 
debug into this if you want.

LieGrue,
strub


> 
>> 
>> LieGrue,
>> strub
>> 
>>> Am 17.07.2020 um 10:11 schrieb Romain Manni-Bucau >> :
>>> 
>>> Hi Mark,
>>> 
>>> If it helps:
>>> 
>>> 1. org.apache.openjpa.kernel.AbstractBrokerFactory#postCreationCallback
>>> only triggers the prepareUnenhancedClasses if preload=true (never the
>> case
>>> right? ;))
>>> 2. Normally classes are loaded in
>>> there org.apache.openjpa.kernel.AbstractBrokerFactory#initializeBroker
>>> *when creating an em, not an emf* so better to have loaded ran the
>>> enhancement early (loadAgent)
>>> 3. Not sure it is done but it can happen 2 has a custom MDR not returning
>>> right the classes - seems code is ok with that. I don't know if it is a
>>> case we want to handle, I don't think it is needed.
>>> 4. There is another case you didn't list where the EMF can be there but
>> no
>>> EM and enhancement should happen ASAP: deserialization ;)
>>> 5. Finally there is a subtle difference between SE and EE case where one
>>> will trigger the agent and the other will just add the transformer in the
>>> classloader through PersistenUnitInfo (and container should handle this
>>> additional properly), I don't know if our agent should be compatible with
>>> it for apploader (jvm one).
>>> 
>>> Romain Manni-Bucau
>>> @rmannibucau  |  Blog
>>>  | Old Blog
>>>  | Github <
>> https://github.com/rmannibucau> |
>>> LinkedIn  | Book
>>> <
>> https://www.packtpub.com/application-development/java-ee-8-high-performance
>>> 
>>> 
>>> 
>>> Le ven. 17 juil. 2020 à 09:35, Mark Struberg 
>> a
>>> écrit :
>>> 
 hi folks!
 
 I'm right now cleaning up old code in our enhancer. Like removing java5
 hacks, etc
 This is also triggered by our recent discussion about _transform and
>> other
 pieces.
 Slowly getting my head into the right space for it. This is way more
 complex than I remembered ;)
 
 So here is my next puzzle:
 
 I've created a new example with 2 classes: Person and Employee extends
 Person
 Plus a persistence.xml with enlisting those 2 classes (makes a
 difference!) and essentially
 
 
 The reason I switched on RuntimeUnenhancedClasses is that this check is
 performed when the BrokerFactory gets created.
 And if the classes are not build-time enhanced, then it fails in
 
>> org.apache.openjpa.enhance.ManagedClassSubclasser#prepareUnenhancedClasses
 if (conf.getRuntimeUnenhancedClassesConstant() !=
 RuntimeUnenhancedClassesModes.SUPPORTED) { ..
 And if you switch to supported, then it will create a new subclass and
 replaces field access to the getters/setters. Sadly
 java.lang.instrument.Instrumentation#retransform cannot be used to add
>> new
 fields or methods. Otherwise we could do a direct full replacement of
>> this
 class.
 
 Fun thing is that later on in
 
>> org.apache.openjpa.persistence.PersistenceProviderImpl#createEntityManagerFactory(java.lang.String,
 java.lang.String, java.util.Map)
 we call loadAgent(factory);
 But what is it supposed to do? Which cases does it serve?
 If the classes did not have been already enhanced at build time nor via
 javaagent they will now be subclassed by ManagedClassSubclasser.
 And if we do not set RuntimeUnenhancedClasses=supported then we will
>> never
 make it so far but blow up way earlier.
 
 Also: it is nice to have that agent attached. But why? All the entity
 classe already have been loaded already during BrokerFactory startup.
>> There
 is no more entity class left over which needs enhancement, isn't?
 
 This is the log I get which reflects what I mean. Happy to share the
>> code
 in a playground branch.
 
 1261  test-classtransform  INFO   [main] openjpa.Enhance - Creating
 subclass and redefining methods for "[class
 org.apache.openjpa.examples.dynenhance.Employee, class
 

Re: A few runtime enhancement/bytecode questions

[Romain Manni-Bucau]

Le ven. 17 juil. 2020 à 10:45, Mark Struberg  a
écrit :

> Both prepareUnenhancedClasses and loadAgent already run in
> createEntityManagerFactory.
>

Not in general, depends the conf and by default i run in none of both with
a standard config for ex for EE.


> crateEntityManager does not trigger enhancement afaict.
>

createEMF does not since the creation of openjpa is lazy so the first
createEM does (in general case one again).


>
> LieGrue,
> strub
>
> > Am 17.07.2020 um 10:11 schrieb Romain Manni-Bucau  >:
> >
> > Hi Mark,
> >
> > If it helps:
> >
> > 1. org.apache.openjpa.kernel.AbstractBrokerFactory#postCreationCallback
> > only triggers the prepareUnenhancedClasses if preload=true (never the
> case
> > right? ;))
> > 2. Normally classes are loaded in
> > there org.apache.openjpa.kernel.AbstractBrokerFactory#initializeBroker
> > *when creating an em, not an emf* so better to have loaded ran the
> > enhancement early (loadAgent)
> > 3. Not sure it is done but it can happen 2 has a custom MDR not returning
> > right the classes - seems code is ok with that. I don't know if it is a
> > case we want to handle, I don't think it is needed.
> > 4. There is another case you didn't list where the EMF can be there but
> no
> > EM and enhancement should happen ASAP: deserialization ;)
> > 5. Finally there is a subtle difference between SE and EE case where one
> > will trigger the agent and the other will just add the transformer in the
> > classloader through PersistenUnitInfo (and container should handle this
> > additional properly), I don't know if our agent should be compatible with
> > it for apploader (jvm one).
> >
> > Romain Manni-Bucau
> > @rmannibucau  |  Blog
> >  | Old Blog
> >  | Github <
> https://github.com/rmannibucau> |
> > LinkedIn  | Book
> > <
> https://www.packtpub.com/application-development/java-ee-8-high-performance
> >
> >
> >
> > Le ven. 17 juil. 2020 à 09:35, Mark Struberg 
> a
> > écrit :
> >
> >> hi folks!
> >>
> >> I'm right now cleaning up old code in our enhancer. Like removing java5
> >> hacks, etc
> >> This is also triggered by our recent discussion about _transform and
> other
> >> pieces.
> >> Slowly getting my head into the right space for it. This is way more
> >> complex than I remembered ;)
> >>
> >> So here is my next puzzle:
> >>
> >> I've created a new example with 2 classes: Person and Employee extends
> >> Person
> >> Plus a persistence.xml with enlisting those 2 classes (makes a
> >> difference!) and essentially
> >> 
> >> 
> >> The reason I switched on RuntimeUnenhancedClasses is that this check is
> >> performed when the BrokerFactory gets created.
> >> And if the classes are not build-time enhanced, then it fails in
> >>
> org.apache.openjpa.enhance.ManagedClassSubclasser#prepareUnenhancedClasses
> >> if (conf.getRuntimeUnenhancedClassesConstant() !=
> >> RuntimeUnenhancedClassesModes.SUPPORTED) { ..
> >> And if you switch to supported, then it will create a new subclass and
> >> replaces field access to the getters/setters. Sadly
> >> java.lang.instrument.Instrumentation#retransform cannot be used to add
> new
> >> fields or methods. Otherwise we could do a direct full replacement of
> this
> >> class.
> >>
> >> Fun thing is that later on in
> >>
> org.apache.openjpa.persistence.PersistenceProviderImpl#createEntityManagerFactory(java.lang.String,
> >> java.lang.String, java.util.Map)
> >> we call loadAgent(factory);
> >> But what is it supposed to do? Which cases does it serve?
> >> If the classes did not have been already enhanced at build time nor via
> >> javaagent they will now be subclassed by ManagedClassSubclasser.
> >> And if we do not set RuntimeUnenhancedClasses=supported then we will
> never
> >> make it so far but blow up way earlier.
> >>
> >> Also: it is nice to have that agent attached. But why? All the entity
> >> classe already have been loaded already during BrokerFactory startup.
> There
> >> is no more entity class left over which needs enhancement, isn't?
> >>
> >> This is the log I get which reflects what I mean. Happy to share the
> code
> >> in a playground branch.
> >>
> >> 1261  test-classtransform  INFO   [main] openjpa.Enhance - Creating
> >> subclass and redefining methods for "[class
> >> org.apache.openjpa.examples.dynenhance.Employee, class
> >> org.apache.openjpa.examples.dynenhance.Person]". This means that your
> >> application will be less efficient than it would if you ran the OpenJPA
> >> enhancer.
> >> 1412  test-classtransform  INFO   [main] openjpa.Runtime - OpenJPA
> >> dynamically loaded the class enhancer. Any classes that were not
> enhanced
> >> at build time will be enhanced when they are loaded by the JVM.
> >>
> >>
> >> LieGrue,
> >> strub
> >>
> >>
>
>

Re: A few runtime enhancement/bytecode questions

[Mark Struberg]

Both prepareUnenhancedClasses and loadAgent already run in 
createEntityManagerFactory.
crateEntityManager does not trigger enhancement afaict.

LieGrue,
strub

> Am 17.07.2020 um 10:11 schrieb Romain Manni-Bucau :
> 
> Hi Mark,
> 
> If it helps:
> 
> 1. org.apache.openjpa.kernel.AbstractBrokerFactory#postCreationCallback
> only triggers the prepareUnenhancedClasses if preload=true (never the case
> right? ;))
> 2. Normally classes are loaded in
> there org.apache.openjpa.kernel.AbstractBrokerFactory#initializeBroker
> *when creating an em, not an emf* so better to have loaded ran the
> enhancement early (loadAgent)
> 3. Not sure it is done but it can happen 2 has a custom MDR not returning
> right the classes - seems code is ok with that. I don't know if it is a
> case we want to handle, I don't think it is needed.
> 4. There is another case you didn't list where the EMF can be there but no
> EM and enhancement should happen ASAP: deserialization ;)
> 5. Finally there is a subtle difference between SE and EE case where one
> will trigger the agent and the other will just add the transformer in the
> classloader through PersistenUnitInfo (and container should handle this
> additional properly), I don't know if our agent should be compatible with
> it for apploader (jvm one).
> 
> Romain Manni-Bucau
> @rmannibucau  |  Blog
>  | Old Blog
>  | Github  |
> LinkedIn  | Book
> 
> 
> 
> Le ven. 17 juil. 2020 à 09:35, Mark Struberg  a
> écrit :
> 
>> hi folks!
>> 
>> I'm right now cleaning up old code in our enhancer. Like removing java5
>> hacks, etc
>> This is also triggered by our recent discussion about _transform and other
>> pieces.
>> Slowly getting my head into the right space for it. This is way more
>> complex than I remembered ;)
>> 
>> So here is my next puzzle:
>> 
>> I've created a new example with 2 classes: Person and Employee extends
>> Person
>> Plus a persistence.xml with enlisting those 2 classes (makes a
>> difference!) and essentially
>> 
>> 
>> The reason I switched on RuntimeUnenhancedClasses is that this check is
>> performed when the BrokerFactory gets created.
>> And if the classes are not build-time enhanced, then it fails in
>> org.apache.openjpa.enhance.ManagedClassSubclasser#prepareUnenhancedClasses
>> if (conf.getRuntimeUnenhancedClassesConstant() !=
>> RuntimeUnenhancedClassesModes.SUPPORTED) { ..
>> And if you switch to supported, then it will create a new subclass and
>> replaces field access to the getters/setters. Sadly
>> java.lang.instrument.Instrumentation#retransform cannot be used to add new
>> fields or methods. Otherwise we could do a direct full replacement of this
>> class.
>> 
>> Fun thing is that later on in
>> org.apache.openjpa.persistence.PersistenceProviderImpl#createEntityManagerFactory(java.lang.String,
>> java.lang.String, java.util.Map)
>> we call loadAgent(factory);
>> But what is it supposed to do? Which cases does it serve?
>> If the classes did not have been already enhanced at build time nor via
>> javaagent they will now be subclassed by ManagedClassSubclasser.
>> And if we do not set RuntimeUnenhancedClasses=supported then we will never
>> make it so far but blow up way earlier.
>> 
>> Also: it is nice to have that agent attached. But why? All the entity
>> classe already have been loaded already during BrokerFactory startup. There
>> is no more entity class left over which needs enhancement, isn't?
>> 
>> This is the log I get which reflects what I mean. Happy to share the code
>> in a playground branch.
>> 
>> 1261  test-classtransform  INFO   [main] openjpa.Enhance - Creating
>> subclass and redefining methods for "[class
>> org.apache.openjpa.examples.dynenhance.Employee, class
>> org.apache.openjpa.examples.dynenhance.Person]". This means that your
>> application will be less efficient than it would if you ran the OpenJPA
>> enhancer.
>> 1412  test-classtransform  INFO   [main] openjpa.Runtime - OpenJPA
>> dynamically loaded the class enhancer. Any classes that were not enhanced
>> at build time will be enhanced when they are loaded by the JVM.
>> 
>> 
>> LieGrue,
>> strub
>> 
>> 

Re: A few runtime enhancement/bytecode questions

[Romain Manni-Bucau]

PS: 6. when you synchronize mapping, classes are loaded earlier than when
you don't

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book



Le ven. 17 juil. 2020 à 10:11, Romain Manni-Bucau  a
écrit :

> Hi Mark,
>
> If it helps:
>
> 1. org.apache.openjpa.kernel.AbstractBrokerFactory#postCreationCallback
> only triggers the prepareUnenhancedClasses if preload=true (never the case
> right? ;))
> 2. Normally classes are loaded in
> there org.apache.openjpa.kernel.AbstractBrokerFactory#initializeBroker
> *when creating an em, not an emf* so better to have loaded ran the
> enhancement early (loadAgent)
> 3. Not sure it is done but it can happen 2 has a custom MDR not returning
> right the classes - seems code is ok with that. I don't know if it is a
> case we want to handle, I don't think it is needed.
> 4. There is another case you didn't list where the EMF can be there but no
> EM and enhancement should happen ASAP: deserialization ;)
> 5. Finally there is a subtle difference between SE and EE case where one
> will trigger the agent and the other will just add the transformer in the
> classloader through PersistenUnitInfo (and container should handle this
> additional properly), I don't know if our agent should be compatible with
> it for apploader (jvm one).
>
> Romain Manni-Bucau
> @rmannibucau  |  Blog
>  | Old Blog
>  | Github
>  | LinkedIn
>  | Book
> 
>
>
> Le ven. 17 juil. 2020 à 09:35, Mark Struberg 
> a écrit :
>
>> hi folks!
>>
>> I'm right now cleaning up old code in our enhancer. Like removing java5
>> hacks, etc
>> This is also triggered by our recent discussion about _transform and
>> other pieces.
>> Slowly getting my head into the right space for it. This is way more
>> complex than I remembered ;)
>>
>> So here is my next puzzle:
>>
>> I've created a new example with 2 classes: Person and Employee extends
>> Person
>> Plus a persistence.xml with enlisting those 2 classes (makes a
>> difference!) and essentially
>> 
>> 
>> The reason I switched on RuntimeUnenhancedClasses is that this check is
>> performed when the BrokerFactory gets created.
>> And if the classes are not build-time enhanced, then it fails in
>> org.apache.openjpa.enhance.ManagedClassSubclasser#prepareUnenhancedClasses
>> if (conf.getRuntimeUnenhancedClassesConstant() !=
>> RuntimeUnenhancedClassesModes.SUPPORTED) { ..
>> And if you switch to supported, then it will create a new subclass and
>> replaces field access to the getters/setters. Sadly
>> java.lang.instrument.Instrumentation#retransform cannot be used to add new
>> fields or methods. Otherwise we could do a direct full replacement of this
>> class.
>>
>> Fun thing is that later on in
>> org.apache.openjpa.persistence.PersistenceProviderImpl#createEntityManagerFactory(java.lang.String,
>> java.lang.String, java.util.Map)
>> we call loadAgent(factory);
>> But what is it supposed to do? Which cases does it serve?
>> If the classes did not have been already enhanced at build time nor via
>> javaagent they will now be subclassed by ManagedClassSubclasser.
>> And if we do not set RuntimeUnenhancedClasses=supported then we will
>> never make it so far but blow up way earlier.
>>
>> Also: it is nice to have that agent attached. But why? All the entity
>> classe already have been loaded already during BrokerFactory startup. There
>> is no more entity class left over which needs enhancement, isn't?
>>
>> This is the log I get which reflects what I mean. Happy to share the code
>> in a playground branch.
>>
>> 1261  test-classtransform  INFO   [main] openjpa.Enhance - Creating
>> subclass and redefining methods for "[class
>> org.apache.openjpa.examples.dynenhance.Employee, class
>> org.apache.openjpa.examples.dynenhance.Person]". This means that your
>> application will be less efficient than it would if you ran the OpenJPA
>> enhancer.
>> 1412  test-classtransform  INFO   [main] openjpa.Runtime - OpenJPA
>> dynamically loaded the class enhancer. Any classes that were not enhanced
>> at build time will be enhanced when they are loaded by the JVM.
>>
>>
>> LieGrue,
>> strub
>>
>>

Re: A few runtime enhancement/bytecode questions

[Romain Manni-Bucau]

Hi Mark,

If it helps:

1. org.apache.openjpa.kernel.AbstractBrokerFactory#postCreationCallback
only triggers the prepareUnenhancedClasses if preload=true (never the case
right? ;))
2. Normally classes are loaded in
there org.apache.openjpa.kernel.AbstractBrokerFactory#initializeBroker
*when creating an em, not an emf* so better to have loaded ran the
enhancement early (loadAgent)
3. Not sure it is done but it can happen 2 has a custom MDR not returning
right the classes - seems code is ok with that. I don't know if it is a
case we want to handle, I don't think it is needed.
4. There is another case you didn't list where the EMF can be there but no
EM and enhancement should happen ASAP: deserialization ;)
5. Finally there is a subtle difference between SE and EE case where one
will trigger the agent and the other will just add the transformer in the
classloader through PersistenUnitInfo (and container should handle this
additional properly), I don't know if our agent should be compatible with
it for apploader (jvm one).

Romain Manni-Bucau
@rmannibucau  |  Blog
 | Old Blog
 | Github  |
LinkedIn  | Book



Le ven. 17 juil. 2020 à 09:35, Mark Struberg  a
écrit :

> hi folks!
>
> I'm right now cleaning up old code in our enhancer. Like removing java5
> hacks, etc
> This is also triggered by our recent discussion about _transform and other
> pieces.
> Slowly getting my head into the right space for it. This is way more
> complex than I remembered ;)
>
> So here is my next puzzle:
>
> I've created a new example with 2 classes: Person and Employee extends
> Person
> Plus a persistence.xml with enlisting those 2 classes (makes a
> difference!) and essentially
> 
> 
> The reason I switched on RuntimeUnenhancedClasses is that this check is
> performed when the BrokerFactory gets created.
> And if the classes are not build-time enhanced, then it fails in
> org.apache.openjpa.enhance.ManagedClassSubclasser#prepareUnenhancedClasses
> if (conf.getRuntimeUnenhancedClassesConstant() !=
> RuntimeUnenhancedClassesModes.SUPPORTED) { ..
> And if you switch to supported, then it will create a new subclass and
> replaces field access to the getters/setters. Sadly
> java.lang.instrument.Instrumentation#retransform cannot be used to add new
> fields or methods. Otherwise we could do a direct full replacement of this
> class.
>
> Fun thing is that later on in
> org.apache.openjpa.persistence.PersistenceProviderImpl#createEntityManagerFactory(java.lang.String,
> java.lang.String, java.util.Map)
> we call loadAgent(factory);
> But what is it supposed to do? Which cases does it serve?
> If the classes did not have been already enhanced at build time nor via
> javaagent they will now be subclassed by ManagedClassSubclasser.
> And if we do not set RuntimeUnenhancedClasses=supported then we will never
> make it so far but blow up way earlier.
>
> Also: it is nice to have that agent attached. But why? All the entity
> classe already have been loaded already during BrokerFactory startup. There
> is no more entity class left over which needs enhancement, isn't?
>
> This is the log I get which reflects what I mean. Happy to share the code
> in a playground branch.
>
> 1261  test-classtransform  INFO   [main] openjpa.Enhance - Creating
> subclass and redefining methods for "[class
> org.apache.openjpa.examples.dynenhance.Employee, class
> org.apache.openjpa.examples.dynenhance.Person]". This means that your
> application will be less efficient than it would if you ran the OpenJPA
> enhancer.
> 1412  test-classtransform  INFO   [main] openjpa.Runtime - OpenJPA
> dynamically loaded the class enhancer. Any classes that were not enhanced
> at build time will be enhanced when they are loaded by the JVM.
>
>
> LieGrue,
> strub
>
>

A few runtime enhancement/bytecode questions

[Mark Struberg]

hi folks!

I'm right now cleaning up old code in our enhancer. Like removing java5 hacks, 
etc
This is also triggered by our recent discussion about _transform and other 
pieces.
Slowly getting my head into the right space for it. This is way more complex 
than I remembered ;)

So here is my next puzzle:

I've created a new example with 2 classes: Person and Employee extends Person
Plus a persistence.xml with enlisting those 2 classes (makes a difference!) and 
essentially


The reason I switched on RuntimeUnenhancedClasses is that this check is 
performed when the BrokerFactory gets created.
And if the classes are not build-time enhanced, then it fails in 
org.apache.openjpa.enhance.ManagedClassSubclasser#prepareUnenhancedClasses
if (conf.getRuntimeUnenhancedClassesConstant() != 
RuntimeUnenhancedClassesModes.SUPPORTED) { ..
And if you switch to supported, then it will create a new subclass and replaces 
field access to the getters/setters. Sadly 
java.lang.instrument.Instrumentation#retransform cannot be used to add new 
fields or methods. Otherwise we could do a direct full replacement of this 
class.

Fun thing is that later on in 
org.apache.openjpa.persistence.PersistenceProviderImpl#createEntityManagerFactory(java.lang.String,
 java.lang.String, java.util.Map)
we call loadAgent(factory);
But what is it supposed to do? Which cases does it serve?
If the classes did not have been already enhanced at build time nor via 
javaagent they will now be subclassed by ManagedClassSubclasser.
And if we do not set RuntimeUnenhancedClasses=supported then we will never make 
it so far but blow up way earlier.

Also: it is nice to have that agent attached. But why? All the entity classe 
already have been loaded already during BrokerFactory startup. There is no more 
entity class left over which needs enhancement, isn't?

This is the log I get which reflects what I mean. Happy to share the code in a 
playground branch.

1261  test-classtransform  INFO   [main] openjpa.Enhance - Creating subclass 
and redefining methods for "[class 
org.apache.openjpa.examples.dynenhance.Employee, class 
org.apache.openjpa.examples.dynenhance.Person]". This means that your 
application will be less efficient than it would if you ran the OpenJPA 
enhancer.
1412  test-classtransform  INFO   [main] openjpa.Runtime - OpenJPA dynamically 
loaded the class enhancer. Any classes that were not enhanced at build time 
will be enhanced when they are loaded by the JVM.


LieGrue,
strub