Skip to content

Public API surface #808

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

Merged
merged 25 commits into from
Aug 31, 2020
Merged

Public API surface #808

merged 25 commits into from
Aug 31, 2020

Conversation

bart-degreed
Copy link
Contributor

This PR is the result of reviewing the publicly exposed types and members of the JsonApiDotNetCore library and their doc-comments. Most notable change is a reorganization of namespaces.

The top-level Internal namespace is gone and has been replaced with multiple Internal sub-namespaces at deeper levels.
The meaning of Internal in the namespace path is that we preserve the right to take breaking changes there without introducing a new major version. Use at your own risk.
I also removed Contract sub-namespaces (containing only interfaces) and aimed to put them next to their implementation where possible.

New hierarchy:

  • Configuration
    Contains types related to bootstrapping and configuration in a broad sense. This includes options, AddJsonApi and UseJsonApi, but also the resource graph, building it and various types related to dependency injection.
  • Controllers
    Base classes for controllers, along with attributes to put on them.
  • Errors
    Reusable exceptions that have a json:api functional representation, such as resource-not-found, method-not-allowed, invalid-query-string etc. Note this is not the place to collect all Exception classes. There are others which exist closely to their functional usage and are for internal use.
  • Hooks.Internal
    Mostly left as-is, this is still experimental and subject to change.
  • Middleware
    The middleware handler itself and various filters, routing conventions, formatters etc that are plugged into the ASP.NET Core request pipeline.
  • Queries.Expressions
    Contains the set of intermediate QueryExpression objects that separate json:api service from IQueryable.
  • Queries.Internal.Parsing
    Contains text parser implementations (coming from query strings and resource definitions).
  • Queries.Internal.QueryableBuilding
    Contains the translation from QueryExpression to IQueryable.
  • QueryStrings
    Interfaces for handling query strings.
  • QueryStrings.Internal
    Implementations for handling query strings.
  • Repositories
    Repository interfaces and the EF Core-based implementation, along with some data-access related types.
  • Resources
    The place for IIdentifyable and ResourceDefinition, along with some internals.
  • Resources.Annotations
    Attributes to put on resources and their members, such as Attr, HasOne, Links, AttrCapabilities etc.
  • Serialization
    The root for everything related to serialization. Removed sub-namespaces for Server/Client because Server is our core business and Client is a sideways feature.
  • Serialization.Building
    The types that take a TResource and produce a ResourceObject from it.
  • Serialization.Client
    Put existing types from Client (intended exclusively for the reverse serialization direction) here.
  • Serialization.Objects
    The set of types we feed into Newtonsoft.Json, such as Document, Error, ResourceLinks, RelationshipEntry etc.
  • Services
    Interfaces and implementation of the Service Layer in the JADNC architecture. Not the place to put unrelated general-purpose service classes.

After applying this new layout, there were only a handful of public types in Internal namespaces left, so I put them next to the other non-Internal namespaced types.
Due to historic reasons, nearly all classes have always been public which may not be appropriate. I did not review those.

Fixes #734
Fixes #567

Bart Koelman added 22 commits August 21, 2020 12:51
Reviewed generic IEnumerable usage in public signatures (fixes 'Possible multiple enumeration' warning)
Reviewed usage of generic List and Dictionary in public signatures
Removed redundant ToList/ToArray/AsReadOnly calls
Replaced ToList() with ToArray(), which uses a bit more memory but is faster
Moved types into separate files (and fixed filename mismatches)
Unified TypeHelper and TypeExtensions (inlined methods with single usage)
…ause it does not work correctly -- it looks like something from before moving to /docs.
Culture-sensitive string comparions
@bart-degreed bart-degreed requested a review from maurei August 26, 2020 10:10
@maurei
Copy link
Member

maurei commented Aug 31, 2020

Is there a specific reason for not having Controllers.Annotations analoguous to Resources.Annotations? If not, I'd propose to add it for consistency, and also it makes sense for splitting up the nuget package into smaller ones in the future

@maurei
Copy link
Member

maurei commented Aug 31, 2020

IGenericServiceFactory and its implementation are currently in the Configuration namespace. I don't think it belongs there, it is not related to configuration of JADNC. It's a service that is used only in the repository. I think this would be a better namespace

@maurei
Copy link
Member

maurei commented Aug 31, 2020

A significant amount of types in the Configuration namespace is only being used one, using the startup phase. I think we can conceptually differentiate between such startup configuration types and runtime configuration types that are involved in handling a client request.

Startup configuration types used only once (lets come up with a better name):

  • TypeLocator
  • IServiceDiscoveryFacade + implementation
  • JsonApplicationBuilder
  • ApplicationBuilderExtensions
  • IdentifiableTypeCache
  • IInverseRelationships + implementation
  • IResourceGraphBuilder + implementation
  • ResourceDescriptor
  • ServiceCollectionExtensions

@maurei
Copy link
Member

maurei commented Aug 31, 2020

I'm not sure about new Serialization namespace, I am a proponent of the previous setup.

  • Serialization.Building: assuming that "building" refers to constructing Documents and/or ResourceObjects, factoring out the client namespace doesn't make sense conceptually because the types in that namespace also construct these objects. The distinction client/server side made more sense.
  • Keeping in mind that in the future we will be modularizing JADNC into smaller nuget packages, I think the client-side serialization would be a candidate for a separate package (since, as you mentioned, server-side functionality is the main feature). The previous common/client/server namespaces separated the types in a way that made sense in order to be able to readily pull out the client and server sided features into separate packages. With the current setup we would have to reorganise the types again if we want to separate it into different packages.

Copy link
Member

@maurei maurei left a comment

Choose a reason for hiding this comment

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

In general great work! I have a left a few other comments in the main PR thread

@bart-degreed
Copy link
Contributor Author

Is there a specific reason for not having Controllers.Annotations analoguous to Resources.Annotations? If not, I'd propose to add it for consistency, and also it makes sense for splitting up the nuget package into smaller ones in the future

There is none, it slipped my mind. I'll update accordingly.

IGenericServiceFactory and its implementation are currently in the Configuration namespace. I don't think it belongs there, it is not related to configuration of JADNC. It's a service that is used only in the repository. I think this would be a better namespace

IGenericServiceFactory (see its doc-comment) retrieves instances from the D/I container. Since the Configuration namespace contains all D/I setup, I think it belongs there too. Who is using this facility is irrelevant and changes over time.

A significant amount of types in the Configuration namespace is only being used one, using the startup phase. I think we can conceptually differentiate between such startup configuration types and runtime configuration types that are involved in handling a client request.

I've tried this, but the distinction is hard to make. For example, JsonApiOptions is used only during startup, while IJsonApiOptions typically is not, so they would not be in the same namespace. Aside from that, I have two objection:

  1. Where a type is used is bad choice to categorize, because the way components are setup typically promote general reuse. So when their usage changes, their declaring namespace should be unaffected.
  2. In general, I think we should try to keep the namespaces to a minimum for easy discovery. This namespace contains 21 public types and interfaces, which I think is okay and does not require a split-up.

I'm not sure about new Serialization namespace, I am a proponent of the previous setup.

  • Serialization.Building: assuming that "building" refers to constructing Documents and/or ResourceObjects, factoring out the client namespace doesn't make sense conceptually because the types in that namespace also construct these objects. The distinction client/server side made more sense.
  • Keeping in mind that in the future we will be modularizing JADNC into smaller nuget packages, I think the client-side serialization would be a candidate for a separate package (since, as you mentioned, server-side functionality is the main feature). The previous common/client/server namespaces separated the types in a way that made sense in order to be able to readily pull out the client and server sided features into separate packages. With the current setup we would have to reorganise the types again if we want to separate it into different packages.

In my opinion the whole Client serialization support should never have been added, therefore I've tried to isolate it from the JADNC library. There are better solutions in the form of language-independent support for generating client libraries using OpenAPI or C#-specific libraries for dealing with REST/JSON like RestSharp which are way better than what we provide here. If there are some types I've missed that only exist for client serialization, I'd like to move them into the Client namespace. I still keep going back and forth on whether Client should be Internal.Client instead, as it is an unfinished attempt to provide some basic support, which breaks when using JADNC advanced features like get-only properties, injected dependencies on resource classes etc.

@bart-degreed
Copy link
Contributor Author

We've had some offline discussion on the client serialization and agreed to make it internal for now. The reasoning is that for server-to-server or client-to-server scenarios we can do better by exploring OpenAPI-generated client libraries in various languages, and/or explore providing extensibility points for RestSharp (which provides a lot more, such as authentication, connection resiliency, handling errors etc), because calling a json:api service involves more than just serialization (like content negotiation, query string composition and character escaping). For integration tests, I prefer to stay close to the request/response data over abstracting that away, which we do not entirely agree upon.

@maurei maurei merged commit 6f72110 into json-api-dotnet:master Aug 31, 2020
@bart-degreed bart-degreed deleted the public-api-surface branch August 31, 2020 14:58
bart-degreed pushed a commit that referenced this pull request Sep 10, 2020
Corrections in documentation after public API changes from #808
nicolestandifer3 added a commit to nicolestandifer3/DotNet-Core-Json-Api that referenced this pull request Aug 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Review public API surface Read through docs and update (focus on hooks + differences to v3)
2 participants