You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@directory.apache.org by "symphony-enrico (via GitHub)" <gi...@apache.org> on 2024/03/12 15:33:46 UTC
[PR] eTag support with If-Match [directory-scimple]
symphony-enrico opened a new pull request, #541:
URL: https://github.com/apache/directory-scimple/pull/541
(no comment)
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1993888550
> Maybe this is something that should be abstracted out?
In my repository implementation I process version in this way:
```java
/**
* Version field may contain: null (If-Match header is not required), * (all version allowed,
* same as null), a specific version or a comma separated version list (not very
* useful, but it is possible according If-Match specifications)
*/
private List<String> processVersion(String version) {
if (StringUtils.isBlank(version) || "*".equals(version.trim())) {
return null;
}
// Weak entity tag will be not processed correctly,
// as they are prefixed W/. But this is not a problem
// because they should not match for If-Match
return Stream.of(version.split(","))
.filter(StringUtils::isNotBlank)
.map(String::trim)
.map(etag -> etag.replaceAll("^\"|\"$", ""))
.collect(Collectors.toList());
}
```
then, if it is not null, I compare it with version of scim entity.
But if we want change APIs for repository inside framework, I can suggest, instead of:
```java
T update(String id, String version, T resource, Set<AttributeReference> includedAttributes, Set<AttributeReference> excludedAttributes) throws ResourceException;
```
We could pass:
```java
T update(String id, @Nullable List<ETag> ifMatch, T resource, Set<AttributeReference> includedAttributes, Set<AttributeReference> excludedAttributes) throws ResourceException;
```
where ETag POJO could be simply:
```java
public class ETag {
String value;
boolean weak;
}
```
WDYT?
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on code in PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#discussion_r1524509240
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java:
##########
@@ -69,14 +70,14 @@ public interface Repository<T extends ScimResource> {
*
*
* @param id the identifier of the ScimResource to update and persist.
- * @param version an optional version (usually used as an ETag) that can be used to optimize update requests, may be compared against, the current {@code ScimResource.meta.version}.
+ * @param etags optional ETag(s) in 'If-Match' header. If not null, to avoid dirty writing, {@code ScimResource.meta.version} must match one of this set (the set should contain only one element, and it must be not weak)
Review Comment:
BTW I added into the comment the it must be not weak... this also because the eta is generated by scimple framework and it is generated as not weak in https://github.com/apache/directory-scimple/blob/a2e61a994187b09940bfa338248af66f987fd5aa/scim-server/src/main/java/org/apache/directory/scim/server/rest/EtagGenerator.java#L68
However I double checked now and I see that all examples in RFC are weak and in:
https://datatracker.ietf.org/doc/html/rfc7644#section-3.14
> The SCIM protocol supports resource versioning via standard HTTP
> ETags ([Section 2.3 of [RFC7232]](https://datatracker.ietf.org/doc/html/rfc7232#section-2.3)). Service providers MAY **support weak ETags**
> as the preferred mechanism for performing conditional
> retrievals and ensuring that clients do not inadvertently overwrite
> each other's changes, respectively
However, it is strange use weak etag for versioning. Anyway one more reason to dont enforce and let implementation to decide
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1995055299
> @symphony-enrico I like it! I think the `ETag` object makes it clear what it's intent is!
>
> Should the `List` be a `Set`? It probably doesn't matter much in this context, as uniqueness is less of an issue, but it would align with the other collection parameters. (though again, i'm not sure this needs to signal uniqueness). Thoughts?
Hi @bdemers I modified to a set, added a parser for header and modified repository apis according to this...
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-2049279530
> I should have some time later this week to get this merged and a release kicked off. (Well the merge part is easy, but I want to play around with it a little and write up something for the vote and release notes)
Hello @bdemers any news? 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.
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1991994026
Along with this we may want to tweak the text in the `Repository.update` methods, some of the nuance that goes along with this? This value could potentially be a comma separated list, or prefixed with `W/`.
Maybe this is something that should be abstracted out?
As is it's still probably an improvement! But if we need to think about changing the API for this, we can (until we cut the official 1.0)
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-2062601466
@symphony-enrico Sorry again for the delay, I hadn't forgot about this, it's a great addition to SCIMple!
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on code in PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#discussion_r1524470637
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java:
##########
@@ -69,14 +70,14 @@ public interface Repository<T extends ScimResource> {
*
*
* @param id the identifier of the ScimResource to update and persist.
- * @param version an optional version (usually used as an ETag) that can be used to optimize update requests, may be compared against, the current {@code ScimResource.meta.version}.
+ * @param etags optional ETag(s) in 'If-Match' header. If not null, to avoid dirty writing, {@code ScimResource.meta.version} must match one of this set (the set should contain only one element, and it must be not weak)
Review Comment:
IMHO we can let implementation decide. There are too much variables (maybe a list of etag is provided, some weak, other not... how decide? Implementation may decide to discard weak one, check only the other... or may decide to refuse all -after all for dirty write protection, it is very strange to provide a list of version, even if allowed by http protocol- ... and may be it can also decide to dont implement versioning at all... or to enforce it, refusing empty etag parameter or refusing *)
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1996895797
ok to test
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-2003448861
any chances to merge it (and release) if there are no blocking point? Thanks a lot
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1997604195
@symphony-enrico SCIMple's ETag support is an area that I think could be improved. (and this Pull Request is very. appreciated in moving that forward!!)
SCIMple _basically_ has 2 types of resources:
- In Memory (Schema, Configuration, these are created when the application starts, and _likely_ will not change)
- Repository controlled (e.g. the persistence of `ScimUser`/`ScimGroup`)
For the in memory items, the `ETagGenerator` should be used.
For Repository controlled objects, it's up to the implementation, they can use the `ETagGenerator`, but they could also use something else (e.g. functionality tied to a database)
> [!NOTE]
> This was relatively recent change (before the last release). For implementations that don't return all fields based on optimized lookups based on the [`attributes`/`excludedAttributes` params](https://datatracker.ietf.org/doc/html/rfc7644#section-3.4.2.5) using the `ETagGenerator` would have resulted in an _incorrect_ version/etag. For example, if a ScimUser Repository that did NOT return group memberships, that data would be missing for hashing in the `ETagGenerator`.
All that being said, if you have thoughts around this, ways to make this easier, or think my above assumption is wrong. Please let us know!
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on code in PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#discussion_r1523752504
##########
scim-core/src/main/java/org/apache/directory/scim/core/repository/Repository.java:
##########
@@ -69,14 +70,14 @@ public interface Repository<T extends ScimResource> {
*
*
* @param id the identifier of the ScimResource to update and persist.
- * @param version an optional version (usually used as an ETag) that can be used to optimize update requests, may be compared against, the current {@code ScimResource.meta.version}.
+ * @param etags optional ETag(s) in 'If-Match' header. If not null, to avoid dirty writing, {@code ScimResource.meta.version} must match one of this set (the set should contain only one element, and it must be not weak)
Review Comment:
Should we throw an exception if the ETag is weak? Or should we let the implementation decide how to handle it?
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1994423009
@symphony-enrico I like it! I think the `ETag` object makes it clear what it's intent is!
Should the `List` be a `Set`? It probably doesn't matter much in this context, as uniqueness is less of an issue, but it would align with the other collection parameters. (though again, i'm not sure this needs to signal uniqueness). Thoughts?
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "symphony-enrico (via GitHub)" <gi...@apache.org>.
symphony-enrico commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1999224254
> Maybe this is something that should be abstracted out?
thank you, I understand. No, for now I dont have suggestions but I am just discovering the framework...
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-1995338796
This looks great @symphony-enrico!
One last quick comment about the weak etag
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers commented on PR #541:
URL: https://github.com/apache/directory-scimple/pull/541#issuecomment-2007505146
I should have some time later this week to get this merged and a release kicked off.
(Well the merge part is easy, but I want to play around with it a little and write up something for the vote and release notes)
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org
Re: [PR] eTag support with If-Match [directory-scimple]
Posted by "bdemers (via GitHub)" <gi...@apache.org>.
bdemers merged PR #541:
URL: https://github.com/apache/directory-scimple/pull/541
--
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: dev-unsubscribe@directory.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@directory.apache.org
For additional commands, e-mail: dev-help@directory.apache.org