anchela commented on a change in pull request #14: URL: https://github.com/apache/sling-org-apache-sling-repoinit-parser/pull/14#discussion_r782185940
########## File path: src/main/java/org/apache/sling/repoinit/parser/impl/AuthorizableIdUtil.java ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.sling.repoinit.parser.impl; + +import java.util.List; +import java.util.regex.Pattern; +import java.util.stream.Collectors; + +public class AuthorizableIdUtil { + + private static final Pattern regex = Pattern.compile("\\s"); + + private AuthorizableIdUtil() { + // hidden + } + + /** + * Gets the authorizable ID handling cases where the value should be quoted + * (e.g. the authorizable id is not just an alphanumeric string) + * + * @param authorizableId the authorizable ID to get + * @return the (potentially quoted) authorizable id + */ + public static final String forRepoInitString(String authorizableId) { + return !regex.matcher(authorizableId).find() ? authorizableId : "\"" + authorizableId + "\""; + } + + /** + * Gets the authorizable IDs handling cases where the value should be quoted + * (e.g. the authorizable id is not just an alphanumeric string) + * + * @param authorizableIds the authorizable IDs to get + * @return the list of (potentially quoted) authorizable ids + */ + public static final List<String> forRepoInitString(List<String> authorizableIds) { Review comment: same as above ########## File path: src/main/java/org/apache/sling/repoinit/parser/operations/AclGroupBase.java ########## @@ -40,7 +41,7 @@ private final List<String> aclOptions; protected AclGroupBase(List<AclLine> lines) { - this(lines,new ArrayList<String>()); + this(lines,new ArrayList<>()); Review comment: IMHO this is an unrelated change. if that is true please move it to a separate pull request ########## File path: src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipals.java ########## @@ -33,14 +34,15 @@ private final List<String> principals; public SetAclPrincipals(List<String> principals, List<AclLine> lines) { - this(principals,lines,new ArrayList<String>()); + this(principals,lines,new ArrayList<>()); Review comment: see above ########## File path: src/main/javacc/RepoInitGrammar.jjt ########## @@ -771,13 +771,21 @@ void removeFromGroupStatement(List<Operation> result) : <REMOVE> members = principalsList() <FROM> <GROUP> - group = <STRING> + group = authorizableId() { result.add(new RemoveGroupMembers(members, group.image)); } } +Token authorizableId() : Review comment: same as with the utility class. the name of the method is IMO not correct as it is used both for principal names and for authorizable ids. ########## File path: src/main/java/org/apache/sling/repoinit/parser/impl/AuthorizableIdUtil.java ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.sling.repoinit.parser.impl; + +import java.util.List; +import java.util.regex.Pattern; +import java.util.stream.Collectors; + +public class AuthorizableIdUtil { + + private static final Pattern regex = Pattern.compile("\\s"); + + private AuthorizableIdUtil() { + // hidden + } + + /** + * Gets the authorizable ID handling cases where the value should be quoted + * (e.g. the authorizable id is not just an alphanumeric string) + * + * @param authorizableId the authorizable ID to get + * @return the (potentially quoted) authorizable id + */ + public static final String forRepoInitString(String authorizableId) { Review comment: same here, sometimes a principal name is passed which isn't the same as the authorizable id. i would also suggest to use notnull/nullable annotations for the param and the return value and adjust the description accordingly ########## File path: src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipalBased.java ########## @@ -33,14 +34,15 @@ private final List<String> principals; public SetAclPrincipalBased(List<String> principals, List<AclLine> lines) { - this(principals,lines,new ArrayList<String>()); + this(principals,lines,new ArrayList<>()); } public SetAclPrincipalBased(List<String> principals, List<AclLine> lines, List<String> aclOptions) { super(lines,aclOptions); this.principals = Collections.unmodifiableList(principals); } + @Override Review comment: see above ########## File path: src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipals.java ########## @@ -33,14 +34,15 @@ private final List<String> principals; public SetAclPrincipals(List<String> principals, List<AclLine> lines) { - this(principals,lines,new ArrayList<String>()); + this(principals,lines,new ArrayList<>()); } public SetAclPrincipals(List<String> principals,List<AclLine> lines, List<String> aclOptions) { super(lines,aclOptions); this.principals = Collections.unmodifiableList(principals); } + @Override Review comment: see above ########## File path: src/main/java/org/apache/sling/repoinit/parser/impl/AuthorizableIdUtil.java ########## @@ -0,0 +1,52 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.sling.repoinit.parser.impl; + +import java.util.List; +import java.util.regex.Pattern; +import java.util.stream.Collectors; + +public class AuthorizableIdUtil { Review comment: i find the name of this class a bit unfortunate as it is used both for parsing authorizable ids as well as for principal names. second, i would suggest to write a dedicate unit test for the methods of this utility class ########## File path: src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPrincipalBased.java ########## @@ -33,14 +34,15 @@ private final List<String> principals; public SetAclPrincipalBased(List<String> principals, List<AclLine> lines) { - this(principals,lines,new ArrayList<String>()); + this(principals,lines,new ArrayList<>()); Review comment: see above ########## File path: src/main/java/org/apache/sling/repoinit/parser/operations/SetAclPaths.java ########## @@ -41,6 +41,7 @@ public SetAclPaths(List<String> paths,List<AclLine> lines, List<String> aclOptio this.paths = Collections.unmodifiableList(paths); } + @Override Review comment: again, IMHO this change is unrelated to the task at hand. i would prefer if it would got in a separate 'minor cleanup' PR with a dedicated 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
