Copilot commented on code in PR #516:
URL: https://github.com/apache/casbin-jcasbin/pull/516#discussion_r3137743355
##########
src/main/java/org/casbin/jcasbin/main/SyncedEnforcer.java:
##########
@@ -405,6 +405,29 @@ public List<String> getAllNamedRoles(String ptype) {
return runSynchronized(() -> super.getAllNamedRoles(ptype),
getReadWriteLock().readLock());
}
+ /**
+ * getUsers gets the list of users that show up in the current policy.
+ * Users are subjects that are not roles.
+ *
+ * @return all users in policy and grouping rules, excluding roles.
Review Comment:
The Javadoc for `getUsers()` says it returns users "in policy and grouping
rules", but `ManagementEnforcer.getUsers()` computes users from policy subjects
minus roles and does not include subjects that appear only in grouping rules.
Please align the Javadoc with the actual behavior (or adjust the implementation
if grouping-only subjects are intended to be included).
```suggestion
* @return all users in policy rules, excluding roles.
```
##########
src/main/java/org/casbin/jcasbin/model/Model.java:
##########
@@ -470,4 +470,54 @@ private void writeString(StringBuilder s, String sec,
Map<String, String> tokenP
s.append(String.format("%s = %s\n", sec, value));
}
}
+
+ /**
+ * getValuesForFieldInPolicyAllTypes gets all values for a field for all
rules
+ * across all policy types in a section. Duplicated values are removed.
+ *
+ * @param sec the section, "p" or "g".
+ * @param fieldIndex the policy rule's index.
+ * @return all field values across all ptypes.
+ */
+ public List<String> getValuesForFieldInPolicyAllTypes(String sec, int
fieldIndex) {
+ List<String> values = new ArrayList<>();
+
+ Map<String, Assertion> section = model.get(sec);
+ if (section == null) {
+ return values;
+ }
+
+ for (Assertion assertion : section.values()) {
+ for (List<String> rule : assertion.policy) {
+ if (fieldIndex < rule.size()) {
+ values.add(rule.get(fieldIndex));
+ }
+ }
+ }
+
+ return Util.arrayRemoveDuplicates(values);
+ }
+
+ /**
+ * getFieldIndex returns the index of a field in a policy type.
+ * For example, given ptype="p" and field="sub", returns the index
+ * where "p_sub" appears in the tokens array.
+ *
+ * @param ptype the policy type, e.g., "p", "p2"
+ * @param field the field name, e.g., "sub", "obj", "act"
+ * @return the index of the field in the policy rule, or 0 if not found
+ */
+ public int getFieldIndex(String ptype, String field) {
+ String pattern = ptype + "_" + field;
+ Assertion ast = model.get("p").get(ptype);
Review Comment:
`getFieldIndex` can throw a NullPointerException when the model doesn't
contain the "p" section (because `model.get("p").get(ptype)` is dereferenced
before any null checks). Consider first fetching the section map and returning
a sentinel/throwing if it's missing, e.g. `Map<String, Assertion> pSec =
model.get("p");` and guard `pSec == null`.
```suggestion
Map<String, Assertion> pSection = model.get("p");
if (pSection == null) {
return 0;
}
Assertion ast = pSection.get(ptype);
```
##########
src/main/java/org/casbin/jcasbin/main/ManagementEnforcer.java:
##########
@@ -125,6 +125,33 @@ public List<String> getAllNamedRoles(String ptype) {
return model.getValuesForFieldInPolicy("g", ptype, 1);
}
+ /**
+ * getUsers gets the list of users that show up in the current policy.
+ * Users are subjects that are not roles (i.e., subjects that do not appear
+ * as the second element in any grouping policy).
+ *
+ * @return all users in policy, excluding roles.
+ */
+ public List<String> getUsers() {
+ List<String> subjects = getAllSubjects();
+ List<String> roles = getAllRoles();
+ return Util.setSubtract(subjects, roles);
+ }
+
+ /**
+ * getNamedUsers gets the list of users that show up in the current named
policy.
+ * Users are subjects that are not roles.
+ *
+ * @param ptype the policy type, can be "p", "p2", "p3", ..
+ * @return all users in the specified policy type, excluding roles.
+ */
+ public List<String> getNamedUsers(String ptype) {
+ int subjectIndex = model.getFieldIndex(ptype, "sub");
Review Comment:
`getNamedUsers` assumes the policy definition for `ptype` contains a `sub`
field. If `model.getFieldIndex(ptype, "sub")` can’t find it (e.g., ABAC-style
policies like `p2 = sub_rule, obj, act, eft`), the current fallback behavior
will return values from index 0 and misclassify them as users. Consider
explicitly handling the “sub not present” case (return empty list or throw a
clear exception) instead of proceeding with index 0.
```suggestion
int subjectIndex = model.getFieldIndex(ptype, "sub");
if (subjectIndex < 0) {
return Collections.emptyList();
}
```
##########
src/main/java/org/casbin/jcasbin/model/Model.java:
##########
@@ -470,4 +470,54 @@ private void writeString(StringBuilder s, String sec,
Map<String, String> tokenP
s.append(String.format("%s = %s\n", sec, value));
}
}
+
+ /**
+ * getValuesForFieldInPolicyAllTypes gets all values for a field for all
rules
+ * across all policy types in a section. Duplicated values are removed.
+ *
+ * @param sec the section, "p" or "g".
+ * @param fieldIndex the policy rule's index.
+ * @return all field values across all ptypes.
+ */
+ public List<String> getValuesForFieldInPolicyAllTypes(String sec, int
fieldIndex) {
+ List<String> values = new ArrayList<>();
+
+ Map<String, Assertion> section = model.get(sec);
+ if (section == null) {
+ return values;
+ }
+
+ for (Assertion assertion : section.values()) {
+ for (List<String> rule : assertion.policy) {
+ if (fieldIndex < rule.size()) {
+ values.add(rule.get(fieldIndex));
+ }
+ }
+ }
+
+ return Util.arrayRemoveDuplicates(values);
+ }
+
+ /**
+ * getFieldIndex returns the index of a field in a policy type.
+ * For example, given ptype="p" and field="sub", returns the index
+ * where "p_sub" appears in the tokens array.
+ *
+ * @param ptype the policy type, e.g., "p", "p2"
+ * @param field the field name, e.g., "sub", "obj", "act"
+ * @return the index of the field in the policy rule, or 0 if not found
+ */
+ public int getFieldIndex(String ptype, String field) {
+ String pattern = ptype + "_" + field;
+ Assertion ast = model.get("p").get(ptype);
+ if (ast == null || ast.tokens == null) {
+ return 0;
+ }
+ for (int i = 0; i < ast.tokens.length; i++) {
+ if (pattern.equals(ast.tokens[i])) {
+ return i;
+ }
+ }
+ return 0;
Review Comment:
Returning `0` when the field token isn’t found is ambiguous because `0` is a
valid field index and can silently produce incorrect results. For example,
`examples/multiple_policy_definitions_model.conf` defines `p2 = sub_rule, obj,
act, eft` (no `sub`), so `getFieldIndex("p2", "sub")` returns `0` and
`getNamedUsers("p2")` would treat `sub_rule` expressions as user names.
Consider returning `-1` (or throwing an `IllegalArgumentException`) when not
found and updating callers to handle it explicitly.
```suggestion
* @return the index of the field in the policy rule, or -1 if not found
*/
public int getFieldIndex(String ptype, String field) {
String pattern = ptype + "_" + field;
Assertion ast = model.get("p").get(ptype);
if (ast == null || ast.tokens == null) {
return -1;
}
for (int i = 0; i < ast.tokens.length; i++) {
if (pattern.equals(ast.tokens[i])) {
return i;
}
}
return -1;
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
