StackWizardSettings breaks java.util.Map contract #73

nigredo-tori · 2018-09-17T09:17:00Z

It doesn't implement .entrySet() and .values(), making the whole thing unsafe to use with the code that expects a proper Map (e.g. to save the resulting settings into a file). Is there a reason why it's implemented this way? There's an issue (WizardSettings.entrySet throws unsupported operating error #46) about it, but I don't quite get why it was closed. Unless, indeed, StackWizardSettings should only be used for controlling the wizard flow, and not for accumulating the resulting settings. Which doesn't seem to be the case, since the data from the named components winds up inside it.
The object returned by .keySet() is not backed by the StackWizardSettings object, which contradicts the Map documentation:

The set is backed by the map, so changes to the map are reflected in the set, and vice-versa.
Even though some similar conflicts are mentioned in the JavaDoc comments (e.g. the remove method), they still break the contract!

It seems to me that the WizardSettings interface shouldn't extends Map at all, and should instead provide a way to get a Map that would be either a snapshot of, or backed by, the current stack item, as well as a way to get an effective Map snapshot for the whole stack.

The text was updated successfully, but these errors were encountered:

stevesobol · 2018-09-17T15:15:22Z

Hello!

I wasn't involved in the project when #46 was opened (or closed), so I have no response to that, but please let me take a look at this and see what I can do about it. Thanks.

spyhunter99 · 2019-01-26T17:29:34Z

maybe it would be better if it just extends HashMap, that would probably fix this and enable us to delete a bunch of code

spyhunter99 · 2019-01-26T18:38:24Z

ehh on second though, i'm not sure that suggestion would work.

stevesobol · 2019-01-27T05:43:38Z

I'm going to dig into this this coming week. @spyhunter99 if you have any ideas, I want to hear them and could you please do me a favor... email me at steve@lobosstudios.com from an email address you would like to use to communicate about this project. You've emailed me in the past but I have no idea if I still have those emails. And we may need to discuss things that don't make sense to discuss here :) thanks

stevesobol · 2019-02-02T18:03:32Z

So, @spyhunter99 opened #46 and then immediately closed it, but his comment about the StackWizardSettings not being for user settings (? not sure exactly what that means) doesn't seem relevant. StackWizardSettings implements WizardSettings, which extends Map. We should implement those missing methods. This seems, to me, to be the path forward. It doesn't involve making breaking changes to the code. Thoughts? @nigredo-tori?

nigredo-tori · 2019-02-03T02:16:51Z

@stevesobol, as I've said, I think that WizardSettings should not extend Map at all, since not all implementations are valid Map implementations. We can keep some of the Map's methods declared in WizardSettings to avoid breaking too much, but we need to make sure those methods' contracts fit all known implementations (for example, remove works differently for StackWizardSettings).

We should implement those missing methods. This seems, to me, to be the path forward. It doesn't involve making breaking changes to the code.

This doesn't break binary compatibility, but it does break the behavior (again, StackWizardSettings.remove would have to change), which to me sounds even worse - this change won't be caught by the compiler!

stevesobol · 2019-02-03T20:34:52Z

Ok. Let me get to work on this - thanks for your feedback

stevesobol · 2019-02-09T03:16:48Z

so... I have Wizard Settings not implementing Map anymore and there are a few methods I have to implement

stevesobol · 2019-02-10T16:58:09Z

get, put andkeySet were all already implemented in StackWizardSettings, so it was just a question of adding them to the WizardSettings interface.

stevesobol · 2019-02-10T17:04:46Z

FlatWizardSetting inherits from HashMap and broke because it didn't implement the get method. This was my solution:

	@Override
	public Object get(String key) {
		return super.get(key);
	}

stevesobol · 2019-02-10T17:15:18Z

@nigredo-tori I haven't opened a pull request yet. I also haven't tested the changes yet. But I'd like you to look at them.

[branch deleted]

nigredo-tori · 2019-02-11T00:54:18Z

@stevesobol, off the top of my head:

StackWizardSettings.entrySet and StackWizardSettings.values are not implemented, and should be removed.
WizardSettings.put should not be called "an optional operation" in its javadoc - unless there is a valid use case for immutable WizardSettings implementations.
Few of the methods in StackWizardSettings use Object rather than String as the key type (get, containsKey, remove, removeAll); since we're breaking a lot of stuff anyway, we can fix these too.

Also, I can think of a few convenience Map methods that can be reimplemented in WizardSettings as default methods but this will have to wait until we drop Java 7 support. Same with conversion from WizardSettings to Map<String, Object>.

stevesobol · 2019-03-31T21:23:52Z

@nigredo-tori I am still here FYI :) trying to find time to work on this, am working on it today and I'm thinking we get all of these things done, including the convenience methods, and since we have major changes, the next release will be 2.0.0 instead of 1.0.10 as I'd planned. There are still things I need to get done (much better docs, a real website, unit testing). But they can wait imho.

stevesobol · 2019-08-06T19:39:11Z

Haven't forgotten about this. Things have been crazy and I've had to focus on getting new work.

stevesobol · 2019-09-11T19:15:48Z

I'm starting clean. I'm not sure exactly where I was with the changes. The branch I linked to earlier is gone, so I'm going to remove the link to it.

stevesobol · 2019-09-11T22:00:58Z

I'm going to have to update WizardSettings too, as StackWizardSettings extends that class and it extends Map.

stevesobol · 2019-09-11T22:09:22Z

I think I'm going to have to grab the source code for Map and basically create a duplicate class, minus the stuff we do not need. Too many methods to rewrite otherwise.

Thank $DEITY for OpenJDK.

stevesobol · 2019-09-11T22:23:55Z

@nigredo-tori hm. OpenJDK is GPL-licensed which means that we'd also need cjwizard to be GPL licensed if I was to snarf the source code for Map and modify it - and cjwizard has always been Apache and I'd prefer to continue using Apache or switch to MIT. Either license is much more permissive than GPL. Thoughts on how we can deal with these changes? If I don't have WizardSettings extending Map, there are a bunch of useful methods that go away...

nigredo-tori · 2019-09-12T03:02:42Z

@stevesobol,

I think I'm going to have to grab the source code for Map and basically create a duplicate class, minus the stuff we do not need. Too many methods to rewrite otherwise.

Please don't do that. Reimplementing library classes is generally not a good idea. In fact, this very issue is about StackWizardSettings incorrectly implementing Map. 😄

It should be easier and cleaner to solve this with aggregation. For example:

public interface WizardSettings {
  void rollBack();
  void newPage(String id);
  void commit();

  /** Get a {@code Map} with settings for the current page.
    * Mutating the resulting map will mutate the settings.
    */
  Map<String, Object> getCurrentPageSettings();

  /** Build a {@code Map} with settings for all the pages.
    * Mutating the resulting map will not mutate the settings.
    */
  Map<String, Object> buildFullSettings();

  // We can provide convenience wrappers for methods we use the most.

  /** Put a key-value pair into the current page settings.
    * @return previous value for this key on the current page, or {@code null}.
    */
  default Object put(String key, Object value) {
    return getCurrentPageSettings().put(key, value);
  }

  /** @return the value for the key in full settings, or {@code null}. */
  default Object get(String key) {
    return buildFullSettings().get(key);
  }
}

I don't like getCurrentPageSettings returning a mutable backing map - this constrains the implementation too much. However, since this whole interface is written in the mutable style, this should do for now.

spyhunter99 · 2019-09-14T17:08:59Z

use a wrapper pattern instead of copy and paste

…

On Wed, Sep 11, 2019 at 11:02 PM nigredo-tori ***@***.***> wrote: @stevesobol <https://github.com/stevesobol>, I think I'm going to have to grab the source code for Map and basically create a duplicate class, minus the stuff we do not need. Too many methods to rewrite otherwise. Please don't do that. Reimplementing library classes is generally not a good idea. In fact, this very issue is about StackWizardSettings incorrectly implementing Map. 😄 It should be easier and cleaner to solve this with aggregation. For example: public interface WizardSettings { void rollBack(); void newPage(String id); void commit(); /** Get a ***@***.*** Map} with settings for the current page. * Mutating the resulting map will mutate the settings. */ Map<String, Object> getCurrentPageSettings(); /** Build a ***@***.*** Map} with settings for all the pages. * Mutating the resulting map will not mutate the settings. */ Map<String, Object> buildFullSettings(); // We can provide convenience wrappers for methods we use the most. /** Put a key-value pair into the current page settings. * @return previous value for this key on the current page, or ***@***.*** null}. */ default Object put(String key, Object value) { return getCurrentPageSettings().put(key, value); } /** @return the value for the key in full settings, or ***@***.*** null}. */ default Object get(String key) { buildFullSettings().get(key); } } I don't like getCurrentPageSettings returning a mutable backing map - this constrains the implementation too much. However, since this whole interface is written in a mutable style, this should do for now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#73?email_source=notifications&email_token=AAPCIGIGUQK5CJUQ7HUUOFLQJGWNJA5CNFSM4FVODXP2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6QP57A#issuecomment-530644732>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPCIGKIWZ533SPY6CB3MPTQJGWNJANCNFSM4FVODXPQ> .

spyhunter99 · 2019-09-14T19:08:25Z

does this help? #82

stevesobol · 2019-09-14T20:24:08Z

I'm sure it will. I'll check it out asap.

I am so sorry, guys. I'm being stupid about this. Lots going on over here, but that's no excuse.

#73 reworks the map interface issue

spyhunter99 added a commit that referenced this issue Sep 28, 2019

Merge pull request #82 from spyhunter99/feature/#73

de940e7

#73 reworks the map interface issue

spyhunter99 added this to the 1.0.10 milestone Sep 28, 2019

spyhunter99 closed this as completed Sep 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

StackWizardSettings breaks java.util.Map contract #73

StackWizardSettings breaks java.util.Map contract #73

nigredo-tori commented Sep 17, 2018 •

edited

Loading

stevesobol commented Sep 17, 2018

spyhunter99 commented Jan 26, 2019

spyhunter99 commented Jan 26, 2019

stevesobol commented Jan 27, 2019

stevesobol commented Feb 2, 2019

nigredo-tori commented Feb 3, 2019 •

edited

Loading

stevesobol commented Feb 3, 2019

stevesobol commented Feb 9, 2019

stevesobol commented Feb 10, 2019 •

edited

Loading

stevesobol commented Feb 10, 2019

stevesobol commented Feb 10, 2019 •

edited

Loading

nigredo-tori commented Feb 11, 2019

stevesobol commented Mar 31, 2019 •

edited

Loading

stevesobol commented Aug 6, 2019

stevesobol commented Sep 11, 2019

stevesobol commented Sep 11, 2019

stevesobol commented Sep 11, 2019

stevesobol commented Sep 11, 2019 •

edited

Loading

nigredo-tori commented Sep 12, 2019 •

edited

Loading

spyhunter99 commented Sep 14, 2019 via email

spyhunter99 commented Sep 14, 2019

stevesobol commented Sep 14, 2019

StackWizardSettings breaks java.util.Map contract #73

StackWizardSettings breaks java.util.Map contract #73

Comments

nigredo-tori commented Sep 17, 2018 • edited Loading

stevesobol commented Sep 17, 2018

spyhunter99 commented Jan 26, 2019

spyhunter99 commented Jan 26, 2019

stevesobol commented Jan 27, 2019

stevesobol commented Feb 2, 2019

nigredo-tori commented Feb 3, 2019 • edited Loading

stevesobol commented Feb 3, 2019

stevesobol commented Feb 9, 2019

stevesobol commented Feb 10, 2019 • edited Loading

stevesobol commented Feb 10, 2019

stevesobol commented Feb 10, 2019 • edited Loading

nigredo-tori commented Feb 11, 2019

stevesobol commented Mar 31, 2019 • edited Loading

stevesobol commented Aug 6, 2019

stevesobol commented Sep 11, 2019

stevesobol commented Sep 11, 2019

stevesobol commented Sep 11, 2019

stevesobol commented Sep 11, 2019 • edited Loading

nigredo-tori commented Sep 12, 2019 • edited Loading

spyhunter99 commented Sep 14, 2019 via email

spyhunter99 commented Sep 14, 2019

stevesobol commented Sep 14, 2019

nigredo-tori commented Sep 17, 2018 •

edited

Loading

nigredo-tori commented Feb 3, 2019 •

edited

Loading

stevesobol commented Feb 10, 2019 •

edited

Loading

stevesobol commented Feb 10, 2019 •

edited

Loading

stevesobol commented Mar 31, 2019 •

edited

Loading

stevesobol commented Sep 11, 2019 •

edited

Loading

nigredo-tori commented Sep 12, 2019 •

edited

Loading