Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRESOLVER-445] Simplify session handling, move out logic from session builder #383

Merged
merged 8 commits into from
Dec 4, 2023

Conversation

cstamas
Copy link
Member

@cstamas cstamas commented Nov 29, 2023

Simplify session handling (copy is gone), also move out logic from session builder to make it reusable.


https://issues.apache.org/jira/browse/MRESOLVER-445

@cstamas cstamas self-assigned this Nov 29, 2023
@cstamas cstamas marked this pull request as ready for review November 29, 2023 20:06
@cstamas cstamas requested review from gnodet and michael-o November 29, 2023 20:06
As this was "emulated" in def session by setReadOnly()
@gnodet
Copy link
Contributor

gnodet commented Nov 30, 2023

@cstamas have you looked at the possibility to add mutators that would return a new RepositorySession instance instead of modifying the existing one in-place. This would allow preserving the immutability, as this feels it goes a bit backward...

If that does not work, it should be possible to implement a wrapper that would simply delegate to a mutable field.

public class DefaultMutableSession implements MutableRepositorySession {
  private RepositorySession session;
  public DefaultMutableSession(RepositorySession session) {
    this.session = session;
  }
  public void setCache(RepositoryCache cache) {
    this.session = session.withCache(cache);
  }
  ...
}

@cstamas
Copy link
Member Author

cstamas commented Nov 30, 2023

@gnodet Yes, taking step back is actually the point here: to ease migration of code that used Resolver 1.x. I looked at patterns how (non-Maven, as Maven is really "clean" in this respect) Resolver was integrated, and this is the best I could come up, to not discourage upgrades.

My plan is following: DefaultRepositorySystemSession (DRSS) should be gone soon, let's say in 2.1.0 version. In a moment DRSS is gone, MutableSession loses it's role, and is to be removed as well, because then the only way to create session becomes the RepositorySystem#createSessionBuilder() method. Basically at that point, when DRSS is gone, the MutableSession has no role to fulfil (as currently all it does, is "bridges" between old code passing around DRSS and using setters on it, and new code that uses SessionBuilder.

My only problem with this, is that am introducing an interface on public API in 2.0.0 that is meant to be gone in (assuming here) 2.1.0. Should it be deprecated immediately as well? Maybe deprecate whole DRSS class as well (and not only def ctor)?

@cstamas
Copy link
Member Author

cstamas commented Nov 30, 2023

Actually scratch all that above, I changed my mind. Soon will push almost totally opposite change to the commit that introduces MutableSession.

@cstamas cstamas changed the title [MRESOLVER-445] Introduce MutableSession to bridge between to two implementation [MRESOLVER-445] Simplify session handling, move out logic from session builder Dec 2, 2023
@cstamas
Copy link
Member Author

cstamas commented Dec 2, 2023

@gnodet "refurbished" this PR (and JIRA) as it went it totally other direction, but I perso like this simplification. Basically, the "copy" method is gone (was present due Maven core stuff you fixed), making all more straightforward.

Copy link
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I would remove the supplier methods from the builder interface. They seem useless as they will be used once and just a few lines after having been defined.

* @return This session for chaining, never {@code null}.
*/
SessionBuilder setCache(RepositoryCache cache);
SessionBuilder setSessionDataSupplier(Supplier<SessionData> dataSupplier);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is slightly unrelated to this PR, but why do we need a Supplier<SessionData> and not directly a SessionData ? Same for setRepositoryCacheSupplier. I don't think they should be part of the API.
If the user wants to set the cache/session data, he will use setSessionData or setRepositoryCache, and for the default values, we can simply inline the default suppliers when the session is actually built.

Copy link
Member Author

@cstamas cstamas Dec 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Internally supplier is used, and here is why:

  • create one builder
  • create session S1 out of builder
  • create another session S2 out of same builder

This "seemingly" trivial operation without supplier would lead that the S1 and S2 would use same data and cache, leading to hardly debuggable issues. Hence, I switched internally to supplier that on each call provides new instance (of data and cache), but IF user does use builder.setCache(cache) it will become () -> cache supplier, so user intentionally can achieve same result.

I am may overthinking this, but wanted to avoid this situation (one builder used for several session instances).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add a warning about this above to setCache/setData methods, and will merge this PR unless some other concern crops up.

@cstamas cstamas merged commit 9b87550 into apache:master Dec 4, 2023
7 checks passed
@cstamas cstamas deleted the MRESOLVER-445 branch December 4, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants