On Tue, 1 Mar 2022 01:44:44 GMT, Yasser Bazzi <d...@openjdk.java.net> wrote:
>> Hi, could i get a review on this implementation proposed by Stuart Marks, i >> decided to use the >> https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/RandomGenerator.html >> interface to create the default method `asRandom()` that wraps around the >> newer algorithms to be used on classes that do not accept the new interface. >> >> Some things to note as proposed by the bug report, the protected method >> next(int bits) is not overrided and setSeed() method if left blank up to >> discussion on what to do with it. >> >> Small test done on >> https://gist.github.com/YShow/da678561419cda8e32fccf3a27a649d4 > > Yasser Bazzi has updated the pull request incrementally with one additional > commit since the last revision: > > fix wrong identation > > Co-authored-by: ExE Boss <3889017+exe-b...@users.noreply.github.com> Hi, I'm getting back to this after returning from vacation. Some of the suggestions made by various people are good and should be applied to this PR. In particular: * Add a private Random constructor that avoids calling `setSeed`. The suggestion from @liach to have it take a `Void` parameter is a good one. Then, have the wrapper's constructor call `super(null)`. This allows you to get rid of the `initialized` variable, and `setSeed` can simply throw unconditionally. * The code path from `Random::from` to the wrapper can be simplified; it looks like there's an extra method here. Probably just have `Random::from` check if the arg is instanceof Random and return it, otherwise it can call the `RandomWrapper` constructor. * Make sure there's a null check as well, probably in the RW constructor. * Make the wrapper a final class, out of an abundance of caution, to reduce the risk of some serialization attacks. Regarding serialization. As far as I can see, none of the new RandomGenerator implementations are serializable. A possibility would be to make the wrapper "conditionally serializable" in case the RG it contains is serializable; but this is impossible to test since we don't have any serializable RG implementations. Thus it seems pointless for the wrapper to support serialization, and it should be disabled. Don't add `serialVersionUID`, and leave the serial warning suppression. To disable serialization, add these methods to the wrapper class: /** * Throws {@code NotSerializableException}. * @param s the object input stream * @throws NotSerializableException always */ @Serial private void readObject(ObjectInputStream s) throws NotSerializableException { throw new NotSerializableException("not serializable"); } /** * Throws {@code NotSerializableException}. * @param s the object output stream * @throws NotSerializableException always */ @Serial private void writeObject(ObjectOutputStream s) throws NotSerializableException { throw new NotSerializableException("not serializable"); } Here's a suggestion for the spec of the public `from` method. Note, it's not necessary to link to `Random` because this method is inside the Random class. /** * Returns an instance of {@code Random} that delegates method calls to the{@link RandomGenerator} * argument. If the generator is an instance of {@code Random}, it is returned. Otherwise, this method * returns an instance of {@code Random} that delegates all methods except {@code setSeed} to the generator. * The returned instance's {@code setSeed} method always throws {@link UnsupportedOperationException}. * The returned instance is not serializable. * * @param generator the {@code RandomGenerator} calls are delegated to * @return the delegating {@code Random} instance */ ------------- PR: https://git.openjdk.java.net/jdk/pull/7001
