14

I've experienced cases where it would be valuable to restrict access to the API of external libraries and frameworks to prevent negative consequences in the system.

For example, in a SharePoint application it might seem natural to call spList.Items.GetItemById to get a list item, even maybe in a loop, without realizing that this can lead to huge performance problems.

It could also be that we need to forbid the usage of SmtpClient in order to force everyone to use our own class to send email, to ensure that we can properly proxy and mock all emailing when in the testing environment.

Are there any reliable and reasonably straightforward ways to achieve these constraints on external code, except from certain specific places in our own code? It's not necessary to absolutely under every circumstance prevent access to these methods/classes, for example by reflection or just some kind of disabling, it should rather be a strict warning that they should not be used. Preferably forcing the programmer to actively take measures to override these constraints if possible/needed.

Alex
  • 402
  • 4
  • 11
  • 12
    This sounds like an enforcing of an extreme form of coding style (Forbidden to use a particular library call). So to me it raises the prerequisite question of do you do any code reviews, or style checks in the first place? – Peter M Oct 11 '17 at 11:37
  • @PeterM Yes we do, but code reviews are much more expensive and unreliable for these specific types of constrains compared to being able to just enforce this in situ. – Alex Oct 11 '17 at 11:44
  • 3
    Are you hoping to catch and block these calls are *runtime* or *compile-time*? – MetaFight Oct 11 '17 at 11:50
  • 1
    @MetaFight Definitively compile-time – Alex Oct 11 '17 at 11:53
  • What about writing documentation on 'proper' use of your API ? – aybe Oct 11 '17 at 11:55
  • I have no knowledge of share point, but if the performance issues of `GetItemById` are contextual, then only a code review can solve the problem of blessing/not allowing an instance. On the other hand you could do a blanket ban with a check-in hook - but that would also need to be contextual. – Peter M Oct 11 '17 at 11:55
  • 1
    @Aybe But that's the point, it's not our API. It's others APIs. And it's simply not as reliable and efficient to rely on knowledge and documentation, compared to just disabling this in the code somehow. – Alex Oct 11 '17 at 11:57
  • @PeterM Of course everything is contextual, the use case is for when these kinds of problems motivate these kinds of general constraints. For us, a general ban on GetItemById is definitely worth it. But that's just an example. – Alex Oct 11 '17 at 12:00
  • If the issue is contextual then your solution needs to understand the context. So I feel that any automated solution would have to be complex in order to work. And now you have another system to maintain. (in hindsight I think I just rephrased my previous comment) – Peter M Oct 11 '17 at 12:03
  • 1
    @PeterM I don't think is has to be complex at all. I see it just like the general warnings tools like resharper offer. If you want to override a warning about something, you just add a special comment. – Alex Oct 11 '17 at 12:06
  • 1
    So there's your solution .. just re-write resharper! Well less complex .. just write an extension to resharper. – Peter M Oct 11 '17 at 12:12
  • 1
    @PeterM Yeah, that would definitely be one possible solution, albeit with a high initial cost I'd presume. If it's possible to get ReSharper to understand things like that. – Alex Oct 11 '17 at 12:16
  • 1
    You can check the symbol table of your binaries (See: [Microsoft equivalent of the nm command](https://stackoverflow.com/questions/375273/microsoft-equivalent-of-the-nm-command)) – mouviciel Oct 11 '17 at 12:19
  • 2
    Since you're using C#, have you ever heard about [StyleCop](https://github.com/StyleCop) ? You know you can create [custom rules](http://www.developerin.net/a/66-StyleCop/50-Create-custom-rules-using-StyleCop) as you please, right ? – Machado Oct 11 '17 at 12:46
  • 11
    "*Are there any reliable and reasonably straightforward ways to achieve these constraints on external code, except from certain specific places in our own code?*". Yes: write your own [Roslyn Analyzer](https://github.com/dotnet/roslyn/wiki/Getting-Started-Writing-a-Custom-Analyzer-&-Code-Fix) to report accessing certain APIs as a compilation error. – David Arno Oct 11 '17 at 12:52
  • 4
    @Machado, StyleCop is effectively a dead product. It's being replaced with StyleCopAnalyzers, which is built on top of Roslyn. It definitely would not be a good idea to invest time in writing custom StyleCop rules these days. – David Arno Oct 11 '17 at 12:55
  • @DavidArno, that makes sense, thanks for pointing that out. Indeed these are interesting times for the .NET ecosystem. Is this the proper link of StyleCopAnalyzers ? https://github.com/DotNetAnalyzers/StyleCopAnalyzers – Machado Oct 11 '17 at 16:07
  • Two words: [static analysis](http://searchwindevelopment.techtarget.com/definition/static-analysis). [Roslyn](https://github.com/dotnet/roslyn-analyzers) seems to be a specific static analyzer for C#. – bishop Oct 11 '17 at 16:49
  • @Machado Thanks for the suggestion about StyleCop (or its replacement). Maybe it would be possible to implement a simple way to lock down public API's building upon this. – Alex Oct 11 '17 at 20:05
  • I haven't seen your code so I have no idea if this'll work, but can the [`internal`](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/internal) keyword help here? – Cody Oct 11 '17 at 20:48
  • 1
    @bishop Roslyn isn't a static analyzer. It is the new [.NET compiler for C# and VB.NET](https://github.com/dotnet/roslyn), rewritten to allow extensions and plugins. Building off It allows static analyzers to use the actual compiler to do all the language level stuff and not have to try to (often badly) reinvent their own parsers. – Mr.Mindor Oct 11 '17 at 22:36

6 Answers6

26

You can do time-consuming things like writing a wrapper around the external API that leaves out your undesired operations, but nothing beats training and code reviews, because whatever standards or technical measures you put in place, people will find creative ways to get around them.

For example, we have several services written in Scala, and one of the things we ask at code review time is for immutability, but we often communicate that as getting rid of the vars. Someone the other day used a val x: ListBuffer[Boolean] to hold a single mutable variable as the only item in the list. You can't assign another ListBuffer to x, but you can replace the items of the list in place as much as you want. Just as bad as using a var, but sneakier.

In other words, you have to check if people are going around your technical solutions. If those technical solutions are costly and add complexity, you may as well just check that they are coding it correctly.

Karl Bielefeldt
  • 146,727
  • 38
  • 279
  • 479
  • damn that is sneaky! – Krupip Oct 11 '17 at 16:34
  • @snb It's equivalent to what Java does as a hack to get around only being able to return one object/value and not having proper reference arguments; passing an array instead that will have its contents updated. (Some examples: `AtomicMarkableReference.get` and `AtomicStampedReference.get`). – JAB Oct 11 '17 at 17:31
  • 1
    Thank for your answer, but I'm definitely not interested in doing time-consuming complex things like writing wrappers around the external code. That wouldn't probably even help since they can just go to the source. This answer seem to assume that a solution _will_ be costly and add complexity. What about a pure and simple solution? – Alex Oct 11 '17 at 19:56
  • 1
    @Alex add a step to your build script that does "grep -R SmtpClient" and fails the build if it finds anything? – user253751 Oct 11 '17 at 22:38
  • 1
    @Alex the simplest solution is right there: "nothing beats training and code reviews". – Mr.Mindor Oct 11 '17 at 22:41
  • 4
    "nothing beats doing it manually" is right until someone automates it. – Ewan Oct 12 '17 at 10:49
  • @Mr.Mindor But we shouldn't swing our golden hammers around either. There are more then one useful tool in developing software. – Alex Oct 12 '17 at 15:47
  • 1
    @Alex not sure which part of the comment you are referring to as a golden hammer. Your previous comment indicated you didn't want a costly solution or one that adds complexity and asked for a 'pure and simple solution' when the purest/simplest solution was already there in the answer. I realize you want a technical solution, but this is inherently a people problem. And some people problems are not a good fit for technical solutions. – Mr.Mindor Oct 12 '17 at 16:40
  • Any technical solution you find or create will add complexity (maybe not to the program but to your overall process as someone will have to set up rules, and maintain the rules, and ensure that they are applied.) You asked for 'except from certain specific places' This alone is probably going to preclude any 'simple' technical solution at enforcing the constraint. – Mr.Mindor Oct 12 '17 at 16:44
  • @Mr.Mindor Sorry for unclarity, I was referring to the "nothing beats training and code reviews". In general maybe no, but even for special use cases? You are most probably absolutely right that this could be implemented in a messy way with hidden costs, but both of us are ourselves in the business of automation and business support. Problems like these are just like the other problems we are working with everyday. – Alex Oct 12 '17 at 16:50
12

Are there any reliable and reasonably straightforward ways to achieve these constraints on external code, except from certain specific places in our own code?

As the question is specifically about C#, there is a compiler-based solution that can be used here to enforce such rules: Roslyn Analyzers. You could write your own analyzer that reports accessing certain APIs as a compilation error or warning.

An example set of analyzers, that provide lots of example code on writing your own, are the StyleCop Analyzers, which are a replacement for the old StyleCop feature for C#.

Having said that, such automated checks can always be worked around by folk determined to "break the rules". Therefore this approach is not a substitute for code reviews as discussed in Karl Bielefeldt's answer. It can assist with such reviews, but should not replace them.

David Arno
  • 38,972
  • 9
  • 88
  • 121
  • It was never the intention to replace anything else, I was just searching for a specialized tool for my toolbox. – Alex Oct 12 '17 at 16:41
2

By now, there is a specific analyzer available specifically for such a task: https://github.com/DotNetAnalyzers/BannedApiAnalyzer

You have to define a ruleset in BannedSymbols.txt. In your case, BannedSymbols.txt would contain something like

M:Microsoft.SharePoint.SPListItemCollection.GetItemById(System.Int32);Potential performance problems
T:System.Net.Mail.SmtpClient;Use OurCompany.FancyClient instead

During compilation, you would then get warning messages Screenshot of error message

Pang
  • 313
  • 4
  • 7
hofingerandi
  • 121
  • 2
0

To elaborate on the "training and code review" suggestion raised in another answer: since the code you wish to forbid is legal code, you can't count on the compiler preventing it, and you'll have to rely on a later process, the review.

This can (and should) include both manual and automatic review steps:

  • Prepare a checklist of known issues and go over them in your manual code reviews, one by one. Have a recurring meeting to review and update the checklist. Whenever a nasty bug is caught and analyzed, add it to the checklist.

  • Add check in rules to look for known patterns. This can be complicated to write, but for a large project, over time, might be useful. TFS allows you to write rules in C#, and other build systems have their own hooks. Consider using gated builds to reject check-ins that match the pattern. Yes, it slows development, but after a certain project size and complexity, slowing down devs can be a good thing.

Avner Shahar-Kashtan
  • 9,166
  • 3
  • 29
  • 37
0

Karl's answer is 100% correct. There is no way to guarantee conformance. However, in addition to training and code reviews, consider the use of static analysis tools to ensure compliance. (Note: I said "in addition to", as one can bypass those as well in exactly the same way that Karl stated).

The advantage to using static analysis tools is one of removing tedious human code analysis looking for instances of "multiple use of IEnumerable" or whatever performance issue of the week you're looking at (or, at least, that I always feel I'm looking at). This will allow the code reviews and training to focus on more "interesting" issues.

For C#, specifically, I've included some suggestions below. Plug these into your build environment and you're good to go. But, generally, no matter what language you're using, there's a static analysis tool out there somewhere.

Copy/paste straight from the Wikipedia page, use the wiki page for the most recent information and links: https://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis#.NET

  • .NET Compiler Platform (Codename Roslyn) – Open-source compiler framework for C# and Visual Basic .NET developed by Microsoft .NET. Provides an API for analyzing and manipulating syntax.
  • CodeIt.Right – Combines static code analysis and automatic refactoring to best practices which allows automatic correction of code errors and violations; supports C# and VB.NET.
  • CodeRush – A plugin for Visual Studio which alerts users to violations of best practices.
  • FxCop – Free static analysis for Microsoft .NET programs that compiles to CIL. Standalone and integrated in some Microsoft Visual Studio editions; by Microsoft.
  • NDepend – Simplifies managing a complex .NET code base by analyzing and visualizing code dependencies, by defining design rules, by doing impact analysis, and by comparing different versions of the code. Integrates into Visual Studio.
  • Parasoft dotTEST – A static analysis, unit testing, and code review plugin for Visual Studio; works with languages for Microsoft .NET Framework and .NET Compact Framework, including C#, VB.NET, ASP.NET and Managed C++.
  • Sonargraph – Supports C#, Java and C/C++ with a focus on dependency analysis, automated architecture check, metrics and the ability to add custom metrics and code-checkers.
  • StyleCop – Analyzes C# source code to enforce a set of style and consistency rules. It can be run from inside of Microsoft Visual Studio or integrated into an MSBuild project.
Reginald Blue
  • 360
  • 3
  • 10
-1

Maybe the compiler can help you to catch unwanted calls.

Rename classes/methods of code in your own lib that should not be used by external lib clients. Alternativly make classes/methods internal and add internals visible to to those classes that are allowed to use them.

External lib users will get a compile error method/class not found.

Forbidden Classes/methods from public libraries: create the same namespace/class/method in your lib

External lib users will get a compile error because of duplicate class found

[update]

It's not necessary to absolutely under every circumstance prevent access to these methods/classes, for example by reflection or just some kind of disabling, ....

forcing the programmer (... client of the lib...) to actively take measures to override these constraints if possible/needed.

k3b
  • 7,488
  • 1
  • 18
  • 31
  • Downvoting this, not just because it's a nasty hack, but it is easily worked around with C# (which the OP has tagged the question with) using [extern aliases](https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/extern-alias). – David Arno Oct 12 '17 at 10:08