7

Say I have a class ImportantInfo with a public writable property Data. Many classes will read this property but only a few will ever set it. Basically, if you want to update Data you should really know what you're doing.

Is there a pattern I could use to make this explicit other than by documenting it? For example, some way to enforce that only classes that implement IUpdateImportantData can do it (this is just an example)? I'm not talking about security here, but more of a "hey, are you sure you want to do that?" kind of thing.

UPDATE

I've been faced with similar situations before, that's why I tried to keep the question more generic but it seems that a more concrete example is definitely required. So here it goes:

I have a Context class with a a property CurrentYear. Many classes use this Context object and, whenever CurrentYear changes, they need to react (reload themselves, for example). Now, there are some classes that can legitimately change CurrentYear. Of course, you don't want just everybody to change CurrentYear as that has an effect in many other classes. How would you go about that?

  • 1
    bad idea that might work: via reflection, check to see if the calling class implements a required interface and throw an exception if they don't. – Steven Evers Oct 25 '13 at 19:42
  • @SteveEvers And this is how you do it in Java: stackoverflow.com/questions/421280/… – user16764 Oct 25 '13 at 19:44
  • What is the worst case that might happen if data are changed without person being aware of it? Only thing that I think of now is add some additional ceremony, like new class/object instance to change the value. – Euphoric Oct 25 '13 at 21:04
  • 1
    Why not just refuse to give access to the Class to just anybody--anyone who wants to change the data has to make a request of a gateway Class that vets the requests? – Amy Blankenship Oct 26 '13 at 0:34
7

So, you currently have:

public class ImportantInfo {
    // constructor
    public ImportantInfo() { ... }

    // getter
    public Data getData() { ... }

    // dangerous setter
    public void setData(Data d) { ... }
}

Managing Mutability

For one thing, maybe you don't want to include a set method by default. For another, you probably don't want a public constructor. Maybe you already have a factory, but your factory could return an immutable interface (that does not expose the set method). The following interface has a method that returns the mutable implementation, but that requires work (so people won't do it by accident) and the name of the method provides a warning:

public interface ImportantInfo {
    public Data getData();

    // ... expose any other safe methods here ...

    public ImportantInfoImplementation getMutableImplementionButBeCareful();
}

Now use a private constructor for ImportantInfo and a factory method that returns the immutable interface:

public class ImportantInfoImplementation {
    // constructor is now private so people can't instantiate this
    // willy-nilly.
    private ImportantInfoImplementation() { ... }

    // public factory method is now the way to get instances of this class
    // and you only get the interface which doesn't expose the dangerous
    // method.
    public static ImportantInfo of() {
        return new ImportantInfoImplementation();
    }

    // getter is mentioned in the interface
    @Override
    public Data getData() { ... }

    // dangerous setter is hidden by the interface.
    public void setData(Data d) { ... }

    // returns the mutable implementing class.
    @Override
    public ImportantInfoImplementation getMutableImplementionButBeCareful() {
        return this;
    }
}

Now to use the dangerous method, clients have to call:

ImportantInfo ii = ImportantInfoImplementation.of();

// Living free and easy with safe immutable version.
someUntrustedMethod(ii);

// probably not messed up yet!
ii.getData();

// OK, I want it badly enough...
ii.getMutableImplementionButBeCareful().setData(d);

I think @Izkata was shooting for something like the above, but I'm not really sure.

Leveraging Immutability

I'm not sure if your set method exposes some underlying danger, or if it makes the instance of this class unstable. If the former, this won't help you, but if the latter, this may solve your problem: Make your class immutable:

public class ImportantInfo {
    private static final Data data;

    // constructor
    public ImportantInfo(Data d) {
        data = d;
    }

    // getter
    public Data getData() { return d; }

    // no-longer dangers setter returns a new ImportantInfo leaving
    // the old one unchanged.
    public ImportantInfo setData(Data d) {
        return new ImportantInfo(d);
    }
}

Now if you call setData() your old ImportantInfo object is unchanged and the client code gets a new ImportantInfo object reflecting the changed state. Of course, your Data object has to be immutable too for this to work properly. If Data is, or holds a collection of some kind, you may find my tips on making collections immutable in Java to be helpful. Update 2017-09-14: instead of that I've been using Paguro to eliminate or manage most mutation in Java.

Deprecation

If none of that makes you happy, you could deprecate the setData() method so that people have to suppress the warning to use it. I don't like this quite as well, but then that may be because I'm not envisioning the reason why it's bad to call this method.

// Be careful when setting data because
//  1. first reason
//  2. second complication
//  3. ...
@Deprecated
public void setData(Data d) { ... }

Conclusion

In any case, I like the idea of giving people easy access to the safe thing, but make them do more work to get the unsafe thing. However you do this, please explain exactly what to be careful of, and why that is a problem so that people will know what they are getting into before they use it.

  • Not really, but I like your version of the setter more. I was trying to create some way for the important data container to "know" what was setting the data, as mentioned in the question – Izkata Oct 26 '13 at 4:53
  • @GlenPeterson, I've updated the question with a more concrete example. It was you answer that led me to the solution so I'm marking it as the accepted answer, but for the sake of completeness I'm posting my own answer with the approach I'll be taking. – Mike Oct 27 '13 at 1:41
10

Please, for the love of all that is holy, don't ever try to do this. You're completely missing the point of class design and encapsulation.

In OOP, classes have invariants. These are the conditions which must be in place in order for the class to function correctly. Almost every class will have at least some invariants unless it is a dumb "property bag" (AKA Data Transfer Object), and too many of these is considered an anti-pattern (Anemic Domain Model).

But back to the main point. A class's responsibilities are therefore to:

  • Ensure that its initial state is valid, via parameterized constructors and validation.
  • Not trust anything from the outside; validate the arguments to all public and probably all protected methods and property setters.
  • Refuse to execute any operation that would leave it in an invalid state (e.g. either by throwing an exception, adjusting other parts of the state to stay valid, or simply performing a no-op and returning an error status). Or, even better, use the type system to make it a compile-time error to supply an invalid value, e.g. by using an enumeration instead of a string/integer parameter.

This is encapsulation. An object does not permit outside access to anything that is not absolutely required, and where it does provide access, it does so very carefully.

With this in mind, it is therefore evident that any properly-designed class should not care who calls it because it is impossible to do anything dangerous - or at least, anything more dangerous than what the class is actually designed to do. If the class is intended to provide dangerous functionality, then you probably need another level of abstraction, i.e. to design your application so that only trusted callers can get a reference to the dangerous object in the first place.

A common example of the latter is data access. You typically do not provide the actual connection object to user-interface components where you might run the risk of data corruption or worse, SQL injection or other security vulnerabilities. Instead, you provide repository or command/query objects that encapsulate the connection.

Even if you came up with some eye-watering code that could reliably validate the caller - even when running in release mode or with binary-rewriters or interception proxies involved - it could only do so at runtime. Which means that programmers would have no clue that they're doing anything wrong until they actually try to run it, which is a terrible way to treat the poor bastards trying to maintain it later on. You'll also run into a mess of dependencies because you'll need to carry this placeholder interface or attribute all over the place.

Then you have "transitive trust" issues - in other words, the calling class may implement whatever interface you decided on, but do you trust the class that's calling that class? In all likelihood, maintenance programmers will get so frustrated with this limitation that they'll just create a wrapper class which technically meets your requirement, which they can then use without fear of nasty runtime errors.

So please, again, don't ever attempt to validate the caller. Validate the arguments if necessary and design your classes so that it is never necessary to validate the caller.

Interesting fact: The original version of the .NET Framework actually had a system for, essentially, validating the caller and the entire call stack. It was called Code Access Security. The policy system turned out to be so complex, awkward to work with, and generally despised by the .NET community, that Microsoft actually decided to remove it in .NET 4. There's still some traces of CAS itself left, but it's all mostly infrastructure-level stuff around assemblies and AppDomains (sandboxing). The fact that there used to be a bulletproof means of caller validation that was actually removed from the framework should tell you a lot about what the programming community at large thinks of this practice.

  • I completely agree with your all your comments, but as I mentioned in my question I'm not really talking about security here. I don't want to validate the caller, I just want the next programmer modifying the code to be sure that he really needs to set the data... – Mike Oct 27 '13 at 1:19
  • 4
    @Mike: I somehow knew you'd respond with that. But don't you see that it's not a meaningful distinction? Have you ever seen a user actually click "No" to Windows' User Account Control? Why not just have a bool reallyDoIt parameter? I'm being facetious of course - it's ineffective for the same reasons that true security is. The net result is waste a lot of programmer time on debugging stupid issues, and eventually they'll get so fed up that they'll either refactor out your "confirmation" or write a wrapper to hide it. Don't do this. Design your class properly and it won't be an issue. – Aaronaught Oct 27 '13 at 14:04
  • @Aarongaught, it seems to me that it seemed to you that what I wanted was to validate the user of the class. What I was asking was how to restrict who can set and get the value vs those who can only get it. If you see my own answer you'll see that, so I guess we're pretty much in the same page. – Mike Oct 27 '13 at 21:21
  • 2
    @Mike: Huh? Where did you get that from? Where in this answer did I mention anything about the user? This whole answer has been dedicated to trying to explain to you that visibility modifiers (public, private, etc.) are the correct way of controlling who has access to a particular operation. If you are finding the need for anything more granular than that, your design is flawed. – Aaronaught Oct 27 '13 at 23:59
  • 1
    @Aarongaught, sorry for the lapsus, I meant the caller, not the user. – Mike Oct 28 '13 at 3:05
2

One thing sort of comes to mind, if you switch to a private member and expose getters/setters.. Bear in mind I've never done .NET and am out of practice with Java, but this is the pseudocode I'm thinking of:

class SomeFickleClass {
   public interface IUpdateImportantData {
      public Data getData();
   }

   private Data myData;

   public function setData(IUpdateImportantData Thing) {
      myData = Thing.getData();
   }
   public function getData() {
      return myData;
   }
}

And usage could be along the lines of:

final Data newData = ....
fickleClassInstance.setData(new SomeFickleClass.IUpdateImportantData() {
   public Data getData() {
      return newData;
   }
});

So this does force an interface that requires IUpdateImportantData to do the update, and can do it on the fly with an anonymous class instance. It's also a little awkward, which should discourage using it often.

  • And by "out of practice", I mean Java 5/6-ish in addition to not having used it for a while. There may well be better ways to do this now. – Izkata Oct 25 '13 at 20:24
1

Based on @GlenPeterson's answer here's what I think I'm gonna do. I'll have two classes:

  • Context. This class is mutable and provides a SetData method.
  • ContextObserver. This class has a private reference to Context, it listens for modifications in Context and whenever Context changes it will execute whatever method you tell it to execute. It also has a method GetContext which returns an immutable version of Context.

So, those who really need to modify the data in Context get a reference to the Context object, while those who only consume the data get a referece to the ContextObserver class.

Plain and simple, if you want to make a class readonly for certain consumers, just give them a readonly wrapper over the original class.

  • Nice. Yes, you want to register "Listeners" with your Context so that they can react when the date changes without doing any polling. – GlenPeterson Oct 27 '13 at 12:09
  • Despite my misgivings about the question, this is essentially the correct approach, so +1. Another way would be to have the context implement an IContext (writable) as well as IReadOnlyContext interface, and have your Context class implement both. It accomplishes the same thing, without the need for an additional "wrapper" class. – Aaronaught Oct 28 '13 at 0:01
0

I was trying to create some way for the important data container to "know" what was setting the data, as mentioned in the question - Izkata

Then I think this is what you want:

public interface IUpdateImportantData {
    public Data newDataValue();
}

public class ImportantInfo {
    // constructor
    public ImportantInfo() { ... }

    // getter
    public Data getData() { ... }

    // safer setter
    public void setData(IUpdateImportantData iuid) {
        // instanceof always returns true when used on null
        if (iuid == null) {
            throw new IllegalArgumentException("Can't set data with null");
        }
        if ( (iuid instanceOf OkClass1) ||
             (iuid instanceOf OkClass2) ) {
            this.data = iuid.newDataValue();
        } else {
            throw new IllegalArgumentException("That class is not allowed to change data");
        }
    }
}

Here's a usage example:

public OkClass2 implements IUpdateImportantData {
    @Override
    public Data newDataValue() { return Data(3); }
}

public OtherClass implements IUpdateImportantData {
    @Override
    public Data newDataValue() { return Data(3); }
}

ImporatntInfo ii = new ImportantInfo();
OkClass2 oc2 = new OkClass2();
ii.setData(oc2); // works.

OtherClass oc1 = new OtherClass();
ii.setData(oc1); // throws exception.

I think I have now answered your question, but this is wrapping a problem with more complexity. It is always superior to fix the underlying problem.

Sometimes you have to interface with some code or service that you cannot control. If you can't fix the underlying problem, you can design an API that allows only valid uses of the underlying difficulty without caring about who is using that API. Preventing invalid usES is far superior to preventing invalid usERS. Can you:

  • Make ImportantInfo a private inner class of the one class that needs to access it?

  • Put ImportantInfo in the same package (folder) as the few classes that need to access it and give setData() default access (package-scope, no-modifier specified)?

  • Split ImportantInfo into 2 classes, one that holds the safe stuff, and one that just protects the unsafe stuff?

  • Redesign ImportantInfo to prevent the dangers that come with modifying the underlying data?

  • Make the underlying data unmodifiable. Inject new versions of Data as needed without modifying the original.

Really, I could give better suggestions if I understood exactly what was dangerous about modifying Data. If you understand that, then the best solution will probably present itself without my help. If not, then you should probably do some experimenting to figure out the root cause of the problem and solve that instead of building layers of complexity around it.

0

It seems like a worrisome approach if you've got several other classes that know how to poke inside an instance of ImportantInfo to update the Data, not because it is safe or otherwise, but rather because you've then got more of a violation of the Law of Demeter.

It would instead be better to put the update code inside the class and have the other classes just pass in instructions on what update is to be performed. In the simple case, these instructions are just arguments to a method, but if you want to do something more complex then the instructions can be packaged up in a little class of their own (which probably should be as close to functionality-free as possible: it's just a description, a POJO in Java parlance). And the update code itself, because it is inside the class, can be much more easily managed in synch with the rest of the class.


I'd also consider seriously whether you really have to support modifying the state at all. Very often, you can make the object be entirely immutable and instead handle changes by returning a new object instead. This is a more functional-programming approach, and it works really rather well as it means that the non-updating users of the object can assume that they won't have changes sprung on them from behind their backs; I can't overstate just how much easier this makes programming!

If you have to support update from outside, consider giving out a true read-only delegate for the object instead so that consumers can't poke inside by mistake. In Java, I'd consider using the proxy mechanism to do this, and I'd have one of the reader methods return an ID (which the writers would need to present to some other manager class to get the updateable entity). Some frameworks can make this very easy to do…

-1

Unfortunately, I do not believe this is possible. I have tried the same approach and many hours of searching but was unsuccessful. The worse part is that comments for use of documentation can become a messy as well. I will do more research however and see what I can find.

-2

You can override and mark methods with the @RestResource(exported = false).

These methods are

T findOne(ID id);
Iterable<T> findAll();
Iterable<T> findAll(Iterable<ID> ids);

You will be getting the 405 Method Not Allowed HTTP status for all GET requests to the repository.

  • 2
    This has nothing to do with the question. Why do you assume OP is talking about a REST api, and not the innards of his project? – Caleth Sep 14 '17 at 11:40

protected by gnat Sep 14 '17 at 13:23

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.