Skip to content
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

refactor: wallet repository search methods #3931

Merged
merged 147 commits into from
Sep 9, 2020

Conversation

rainydio
Copy link
Contributor

@rainydio rainydio commented Aug 5, 2020

Search can be described as searching through API resources. Its functions are tailored for API use and aren't part of wallet repository anymore. A lot changes were made to the way controllers, resources, and search services interact together. Most of them are breaking. Please point me to other repositories I may have to update.

API methods related to wallet repository were massively updated while methods related to blocks and transactions not so much. Similar concepts are implemented differently yet use the same name which might be confusing.

Example

Searching entities from magistrate package is perfect simple example to dissect.

Resource

To start create EntityResource type definition:

packages/core-magistrate-api/src/resources/entity.ts

export type EntityResource = {
  id: string;
  publicKey: string;
  address: string;
  isResigned: boolean;
  type: Enums.EntityType;
  subType: Enums.EntitySubType;
  data: Interfaces.IEntityAssetData;
};

Resources aren't transformer classes anymore. They are types that are eventually returned by API controllers.

In addition to EntityResource three other resource classes WalletResource, DelegateResource, and LockResource were replaced with similar type definition. While BlockResource and TransactionResource were left untouched, they are transformer classes still.

Criteria

When searching EntityCriteria type is used to filter EntityResource. Generic Contracts.Search.StandardCriteriaOf type is used to convert resource type into criteria type:

packages/core-magistrate-api/src/resources/entity.ts

export type EntityCriteria = Contracts.Search.StandardCriteriaOf<
  EntityResource
>;

// EntityResource

Standard criteria properties carry AND meaning, array items OR. Here is an example resigned businesses and developers criteria:

const criteria = {
  isResigned: true,
  type: [Enums.EntityType.Business, Enums.EntityType.Developer],
};

// entity.isResigned &&
//   (entity.type === Enums.EntityType.Business ||
//     entity.type === Enums.EntityType.Developer);

Search service

EntitySearchService connects the two. It accepts EntityCriteria and returns EntityResource:

packages/core-magistrate-api/src/services/entity-search-service.ts

@Container.injectable()
export class EntitySearchService {
  public getEntity(entityId: string): EntityResource | undefined {
    throw new Error("Not implemented");
  }

  public getEntitiesPage(
    pagination: Contracts.Search.Pagination,
    sorting: Contracts.Search.Sorting,
    ...criterias: EntityCriteria[]
  ) {
    throw new Error("Not implemented");
  }
}

getEntityResourceFromWallet

Both getEntity and getEntitiesPage rely on private getEntityResourceFromWallet method to create resource object:

packages/core-magistrate-api/src/services/entity-search-service.ts

@Container.injectable()
export class EntitySearchService {
  // getEntity
  // getEntitiesPage

  private getEntityResourceFromWallet(
    wallet: Contracts.State.Wallet,
    entityId: string
  ): EntityResource {
    const entity = wallet.getAttribute<IEntitiesWallet>("entities")[entityId];

    return {
      id: entityId,
      address: wallet.address,
      publicKey: wallet.publicKey,
      isResigned: !!entity.resigned,
      type: entity.type,
      subType: entity.subType,
      data: entity.data,
    };
  }
}

Previously wallet and delegate resources had meaningful transform method, single wallet can be transformed into single delegate. Wallet repository had to improvise when searching through delegates because it didn't had delegate object at hand which was created at the very end of controller. Single wallet cannot be transformed into single lock or entity, there might be few. Their transform methods were empty. Creation happened within wallet repository itself breaking isolation. Wallet repository was aware of core-magistrate.

This getEntityResourceFromWallet method feels like transform and is in core-magistrate-api package, but it's semantically closer to the way locks and entities were created.

getEntity

getEntity implementation is straightforward. It looks for wallet in "entities" index and uses getEntityResourceFromWallet to return resource:

packages/core-magistrate-api/src/services/entity-search-service.ts

@Container.injectable()
export class EntitySearchService {
  public getEntity(entityId: string): EntityResource | undefined {
    if (this.walletRepository.hasByIndex("entities", entityId)) {
      const wallet = this.walletRepository.findByIndex("entities", entityId);
      return this.getEntityResourceFromWallet(wallet, entityId);
    } else {
      return undefined;
    }
  }

  // getEntitiesPage
  // getEntityResourceFromWallet
}

*getEntities

To save resources (isn't implemented downstream yet) getEntitiesPage is accompanied by private generator method *getEntities. It creates and immediately filters out entities that do not match criteria:

packages/core-magistrate-api/src/services/entity-search-service.ts

@Container.injectable()
export class EntitySearchService {
  // getEntity(entityId: string)
  // getEntitiesPage
  // getEntityResourceFromWallet

  private *getEntities(
    ...criterias: EntityCriteria[]
  ): Iterable<EntityResource> {
    const entities = this.walletRepository.getIndex("entities").entries();

    for (const [entityId, wallet] of entities) {
      const entityResource = this.getEntityResourceFromWallet(wallet, entityId);

      if (
        this.standardCriteriaService.testStandardCriterias(
          entityResource,
          ...criterias
        )
      ) {
        yield entityResource;
      }
    }
  }
}

Services.Search.StandardCriteriaService knows how to test if T matches standard criteria Contracts.Search.StandardCriteriaOf<T>.

getEntitiesPage

Finally the last method that returns page of entities. It sets default sorting and then feeds filtered Iterable<EntityResource> into pagination service for sorting and slicing:

packages/core-magistrate-api/src/services/entity-search-service.ts

@Container.injectable()
export class EntitySearchService {
  // getEntity(entityId: string)

  public getEntitiesPage(
    pagination: Contracts.Search.Pagination,
    sorting: Contracts.Search.Sorting,
    ...criterias: EntityCriteria[]
  ): Contracts.Search.ResultsPage<EntityResource> {
    sorting = [...sorting, { property: "data.name", direction: "asc" }];

    return this.paginationService.getPage(
      pagination,
      sorting,
      this.getEntities(...criterias)
    );
  }

  // getEntityResourceFromWallet
  // *getEntities
}

Services.Search.PaginationService can sort over multiple properties. It currently does not take advantage of generators to save memory and is merely Array.from().sort().slice().

Controller validation

Controller index method gets criteria from query string while search gets criteria from payload. Schemas are built from schema object. It is defined in the same file as resource so if one is updated it's difficult to miss the other:

packages/core-magistrate-api/src/resources/entity.ts

// EntityCriteria
// EntityResource

export const entityCriteriaSchemaObject = {
  id: CoreResources.transactionCriteriaSchemaObject.id,
  publicKey: CoreResources.walletCriteriaSchemaObject.publicKey,
  address: CoreResources.walletCriteriaSchemaObject.address,
  isResigned: Joi.boolean(),
  type: Joi.number().allow(
    Enums.EntityType.Business,
    Enums.EntityType.Bridgechain,
    Enums.EntityType.Developer,
    Enums.EntityType.Plugin
  ),
  subType: Joi.number().allow(
    Enums.EntitySubType.None,
    Enums.EntitySubType.PluginCore,
    Enums.EntitySubType.PluginDesktop
  ),
  data: {
    name: Joi.string(),
    ipfsData: Joi.string(),
  },
};

export const entityCriteriaQuerySchema = Joi.object(entityCriteriaSchemaObject);
export const entityCriteriaPayloadSchema = Schemas.createCriteriaPayloadSchema(
  entityCriteriaSchemaObject
);
export const entitySortingSchema = Schemas.createSortingSchema(
  entityCriteriaSchemaObject
);

Controller

index and search methods directly return Contracts.Search.ResultsPage<EntityResource> from getEntitiesPage which suitable for pagination plugin.

show method has to wrap EntityResource into { data: EntityResource }.

@lgtm-com
Copy link

lgtm-com bot commented Aug 5, 2020

This pull request introduces 1 alert when merging 8df4d75 into 08d8c93 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

@rainydio
Copy link
Contributor Author

rainydio commented Sep 4, 2020

I made two new api clients in core-test-framework. One is sending real HTTP requests, another is using server.inject call. Both behave exactly the same, same request and response format. Responses returned by server.inject are actual objects returned by controllers, they are not stringified and then parsed back. Many resources add BigNumber.toFixed calls which are unnecessary and are merely artifacts of poor client. I wrote most tests using rest client (to check real responses) and then switched to inject client to make them run faster.

Testing clients (covering core-test-framework) requires creating new real controller (in __support__) to get real requests going. So last 0.4% percentage coverage is difficult to reach. I will cover it while review is going.

@rainydio rainydio marked this pull request as ready for review September 4, 2020 12:19
@air1one air1one merged commit 17fcaf3 into develop Sep 9, 2020
@ghost ghost deleted the refactor/wallet-repository-search branch September 9, 2020 13:47
@ghost ghost removed the Status: Needs Review label Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants