You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jackrabbit.apache.org by GitBox <gi...@apache.org> on 2021/06/04 09:38:08 UTC
[GitHub] [jackrabbit-filevault] stoerr opened a new pull request #143: JCRVLT-526 PackageProperties timezone format extension
stoerr opened a new pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143
When packages e.g. a creation date with a timezone format +- hhmm or +-hh are entered into the FSPackageRegistry, the date cannot be read by org.apache.jackrabbit.vault.packaging.impl.PackagePropertiesImpl.getCreated() : it returns null since it internally uses org.apache.jackrabbit.util.ISO8601.parse to parse the date, which doesn't support that (compare https://issues.apache.org/jira/browse/JCR-4267). This is a problem, since com.day.jcr.vault:content-package-maven-plugin (I tried versions 0.0.12 and 0.5.4) write package properties using such a format - e.g. 2021-05-26T15:12:21.673+0200 , while org.apache.jackrabbit.util.ISO8601.parse just supports a format like 2021-05-26T15:12:21.673+02:00 .
This extends the PackageProperties to deal with these.
--
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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on a change in pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143#discussion_r646715327
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +0200 that aren't supported by ISO8601.parse
+ // but sometimes produced by maven plugins, compare https://issues.apache.org/jira/browse/JCRVLT-276
+ Matcher splittedDate = TIMEZONE_FIX_PATTERN.matcher(p);
Review comment:
But based on this suggestion I clean up the impl in https://github.com/apache/jackrabbit-filevault/pull/144. @stoerr @jsedding Please have a look
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +0200 that aren't supported by ISO8601.parse
+ // but sometimes produced by maven plugins, compare https://issues.apache.org/jira/browse/JCRVLT-276
+ Matcher splittedDate = TIMEZONE_FIX_PATTERN.matcher(p);
Review comment:
But based on this suggestion I cleaned up the impl in https://github.com/apache/jackrabbit-filevault/pull/144. @stoerr @jsedding Please 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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on a change in pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143#discussion_r646560097
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +0200 that aren't supported by ISO8601.parse
+ // but sometimes produced by maven plugins, compare https://issues.apache.org/jira/browse/JCRVLT-276
+ Matcher splittedDate = TIMEZONE_FIX_PATTERN.matcher(p);
Review comment:
According to https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#ISO_OFFSET_DATE_TIME and https://docs.oracle.com/javase/8/docs/api/java/time/ZoneOffset.html#getId-- only supports the following timezone patterns:
> Z - for UTC (ISO-8601)
> +hh:mm or -hh:mm - if the seconds are zero (ISO-8601)
> +hh:mm:ss or -hh:mm:ss - if the seconds are non-zero (not ISO-8601)
But not something like `+hhmm` or just `+hh` or am I missing something here?
--
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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] stoerr commented on a change in pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
stoerr commented on a change in pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143#discussion_r645558743
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +02:00 that isn't supported by ISO8601.parse
Review comment:
Right, I confused that. 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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] jsedding commented on a change in pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
jsedding commented on a change in pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143#discussion_r646507030
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +0200 that aren't supported by ISO8601.parse
+ // but sometimes produced by maven plugins, compare https://issues.apache.org/jira/browse/JCRVLT-276
+ Matcher splittedDate = TIMEZONE_FIX_PATTERN.matcher(p);
Review comment:
Rather than fixing things here, why not use `GregorianCalendar.from(ZonedDateTime.parse(p, DateTimeFormatter.ISO_OFFSET_DATE_TIME))`? Whether you then still call `ISO8601.parse(p)` before or not is a matter of taste.
--
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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on a change in pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143#discussion_r646560097
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +0200 that aren't supported by ISO8601.parse
+ // but sometimes produced by maven plugins, compare https://issues.apache.org/jira/browse/JCRVLT-276
+ Matcher splittedDate = TIMEZONE_FIX_PATTERN.matcher(p);
Review comment:
According to https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#ISO_OFFSET_DATE_TIME and https://docs.oracle.com/javase/8/docs/api/java/time/ZoneOffset.html#getId-- `DateTimeFormatter.ISO_OFFSET_DATE_TIME` only supports the following timezone patterns:
> Z - for UTC (ISO-8601)
> +hh:mm or -hh:mm - if the seconds are zero (ISO-8601)
> +hh:mm:ss or -hh:mm:ss - if the seconds are non-zero (not ISO-8601)
But not something like `+hhmm` or just `+hh` or am I missing something here?
--
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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin commented on a change in pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143#discussion_r645480727
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +02:00 that isn't supported by ISO8601.parse
Review comment:
the comment is confusing, isn't the pattern only supporting +02 or 0200 (without colon)?
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +02:00 that isn't supported by ISO8601.parse
+ // but sometimes produced by maven plugins, compare JCRVLT-526
Review comment:
the more suitable reference is IMHO https://issues.apache.org/jira/browse/JCRVLT-276
--
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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] stoerr commented on a change in pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
stoerr commented on a change in pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143#discussion_r645559559
##########
File path: vault-core/src/main/java/org/apache/jackrabbit/vault/packaging/impl/PackagePropertiesImpl.java
##########
@@ -211,22 +216,34 @@ public boolean requiresRestart() {
return dependenciesLocations;
}
}
-
+
/**
* {@inheritDoc}
*/
@Override
public Calendar getDateProperty(String name) {
+ Calendar result = null;
try {
// TODO: add timezone if not there?
String p = getProperty(name);
- return p == null
- ? null
- : ISO8601.parse(p);
+ if (p != null) {
+ result = ISO8601.parse(p);
+ if (result == null) {
+ // Perhaps this is due to unusual timezone format +02 or +02:00 that isn't supported by ISO8601.parse
+ // but sometimes produced by maven plugins, compare JCRVLT-526
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:
users@infra.apache.org
[GitHub] [jackrabbit-filevault] kwin merged pull request #143: JCRVLT-526 PackageProperties timezone format extension
Posted by GitBox <gi...@apache.org>.
kwin merged pull request #143:
URL: https://github.com/apache/jackrabbit-filevault/pull/143
--
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:
users@infra.apache.org