Can't reuse SimpleCollection returned by SimpleService

[overheadhunter]

SimpleCollection is AutoCloseable i.e. it should either be used in a try-with-resource statement or wrapped in a class that is itself AutoCloseable.

SimpleService returns a single instance of SimpleCollection here:

secret-service/src/main/java/org/freedesktop/secret/simple/SimpleService.java

Lines 12 to 23 in 0630ae4

	/**
	* Connect to the default collection.
	*/
	public Optional<SimpleCollection> connect() {
	try {
	SimpleCollection keyring = new SimpleCollection();
	return Optional.of(keyring);
	} catch (Exception e) {
	log.error(e.toString(), e.getCause());
	return Optional.empty();
	}
	}

During your unit tests you only use such instances once and dispose them afterwards, thus they work. But the API suggests reusability, which is not provided.

I.e. the following code does close() the SimpleCollection twice:

Optional<SimpleCollection> connection = new SimpleService().connect();
assert connection.isPresent();

try (SimpleCollection collection = connection.get()) {
    ...
} catch (IOException e) { ... }

// at this point the collection is already closed:
try (SimpleCollection collection = connection.get()) {
    ...
} catch (IOException e) { ... }

I suggest to design the API that can be used like this:

try (SimpleService service = SimpleService.create()) { // factory method instead of constructor, as public constructors should ideally not throw any exceptions. attempt to initialize basic dbus connection here.
    try (SimpleServiceSession session = service.createSession()) {
        session.doStuffOnKeyring();
    } catch (IOException e) { ... }

    try (SimpleServiceSession session = service.createSession()) {
        session.doOtherStuffOnKeyring();
    } catch (IOException e) { ... }
} catch (SimpleServiceUnavailableException e) { ... }

[swiesend]

@purejava, @overheadhunter this is what I currently have: develop-2.x.x. Can you both give me feedback on the API?

The SimpleCollection is still around and should be backwards compatible as this point, but I want to remove it entirely.

The new interface is under: de.swiesend.secretservice.functional. One has to start with SecretService.create(). See: https:/swiesend/secret-service/blob/develop-2.x.x/src/test/java/de/swiesend/secretservice/functional/integration/Example.java

• the Optional API is not playing nicely with the Autoclose.
• returning Optional<Secret> is maybe not what we want for encrypt and decrypt:

  secret-service/src/main/java/de/swiesend/secretservice/TransportEncryption.java

  Line 177 in 074ed17

  public Optional<Secret> encrypt(CharSequence plain) {

• @purejava if you find time to add further tests and for the new API I would appreciate this very much!
• there are some TODOs, which I like to have feedback on

Lately I did not find much time for this project, but tomorrow I have some spare time and like to solve this issue for good.

[purejava]

@purejava, @overheadhunter this is what I currently have: develop-2.x.x. Can you both give me feedback on the API?

I took a quick look at your commits / browsed over them. This raised a couple of questions in my head. What is the new API? Is the API change making the connection stuff optional? Anything else?
When you want someone to review the API, one or two sentences what is intended makes a reviewer's life easier, I think.

And I run your Example test class. Discarding the password prompt for "create new keyring", leads to an NPE. This should work already for the code at this point from my perspective.

So, my next question would be, how finished is your code? You write, the API is done, but state below, that there are still issues and the design will change. This confuses me.

I doubt that a review at this time makes sense.

The SimpleCollection is still around and should be backwards compatible as this point, but I want to remove it entirely.

The new interface is under: de.swiesend.secretservice.functional. One has to start with SecretService.create(). See: https:/swiesend/secret-service/blob/develop-2.x.x/src/test/java/de/swiesend/secretservice/functional/integration/Example.java

* [ ]  the `Optional` API is not playing nicely with the `Autoclose`.

* [ ]  returning `Optional<Secret>` is maybe not what we want for `encrypt` and `decrypt`: https:/swiesend/secret-service/blob/074ed179f98bccea8c9bc85a999913673ed3fd45/src/main/java/de/swiesend/secretservice/TransportEncryption.java#L177

* [ ]  @purejava if you find time to add further tests and for the new API I would appreciate this very much!

I am sorry, but I won't find the time to do that.

* [ ]  there are some `TODO`s, which I like to have feedback on

Lately I did not find much time for this project, but tomorrow I have some spare time and like to solve this issue for good.

Good luck. Honestly. To be honest, personally I don't feel any motivation to review an API change to develop the whole lib further, while a ready to merge contribution to current problems for the nowadays API gets ignored, but somehow re-used here.

I'd suggest and prefer to discuss and fix issues raised for this lib in the past, before moving on to new goals.

[overheadhunter]

Good to hear you find some time for this lib again! Before looking at the API or code, I would like to suggest merging #40, because its huge changeset (due to the package renaming) poisons the diff of #32, now that you rebased it upon the former. This makes reviewing it unnecessarily complicated. I would propose to move forward in smaller increments, e.g. like this:

%%{init: { 'gitGraph': {'showBranches': true, 'showCommitLabel':false,'mainBranchName': 'develop'}} }%%
gitGraph
    commit
    branch 40-modularization order: 3
    branch 2.0.0-develop order: 1
    checkout 40-modularization
    commit
    commit
    checkout 2.0.0-develop
    merge 40-modularization tag: "v2.0.0-beta1"
    branch 32-new-api order: 2
    commit
    commit
    checkout 32-new-api
    commit
    commit
    checkout 2.0.0-develop
    merge 32-new-api  tag: "v2.0.0-beta2"

Loading