refactored data fetchers to be more abstract #32

kss-espeo · 2019-07-03T13:53:55Z

Rationale behind this change: not all of the fetchers depend on a REST call (for example random data provider), so it doesn't make sense to put URL in the interface call. Still, we'd like a consistent interface to implement FetcherFactory once random data provider is introduced.

src/domain/common/usecase/FetchDataUseCase.js

tcichowicz · 2019-07-03T14:03:14Z

src/domain/request/Request.js

@@ -11,10 +11,6 @@ class Request {
    this.state = state ? new RequestState(state) : RequestStateFactory.createState(validFrom);
  }

-  getRawUrl() {


We strive for rich domain objects that know their own capabilities. RequestUrlParser was always meant more as an internal utility for Request (in Java we'd give it package scope). Maybe there's some other way? Polymorphism comes to mind but I'm sure there are other possibilities. We could rethink the interface of a Request domain object to better fit our use cases (here specifically FetchDataUseCase).

There are some distinct features (such as getRawUrl) that are going to be only used by some fetchers. I take the point about polymorphism - Request could be an abstraction and we could implement RandomRequest or RestRequest. This might however be redundant if there would be 1-to-1 relation between Request and Fetcher implementation. I think we should revisit this after implementing Random Provider, but for now let's leave it as is.

Maybe we should have different use cases and some kind of strategy to choose from them? I'm reluctant to accept it the way it is now. By "leaving it as is" you mean to not merge this for now?

Maybe we should have different use cases and some kind of strategy to choose from them?

I agree. This PR is a first step for achieving this.

By "leaving it as is" you mean to not merge this for now?

Originally I wanted to merge this asap, then merge random provider once it's ready and then write something like a "strategy picker" refactor you mentioned before.
But if you would like me to, I could do it the hard way and design "strategy" picker before merging random provider :) . That way it would be a little harder to write and much harder to review

I think there's some middle ground between merging this as is and producing a large PR with both the provider and the strategy.

How about we create a RequestExecutorFactory which would create (select?) a use case based on the request. Each executor would return a response given a request. We'd have different request executors:

One that basically does what fetchAndSelectData function from ExecuteReadyRequestsUseCase does - maybe UrlRequestExecutor?

One that can provide a random value based - RandomValueRequestExecutor?

This way we can keep the domain pure and segregate the concerns. UrlRequestExecutor would handle all the URL-related stuff so that the Request does not need to know about it at all.

przemyslawjanpietrzak

Pls merge master to feature branch

.gitignore

…actor/abstractonise-data-fetchers

src/domain/common/port/DataFetcherPort.ts

…s so

src/application/requestExecutor/RequestExecutorFactory.ts

src/domain/common/port/DataFetcherPort.ts

src/application/dataFetcher/AxiosUrlDataFetcherAdapter.ts

src/domain/common/usecase/FetchDataUseCase.ts

src/domain/request/requestExecutor/RequestExecutorFactory.ts

src/domain/request/requestExecutor/RequestExecutorFactory.spec.ts

src/domain/request/requestExecutor/RequestExecutorFactory.ts

kwedrowicz · 2019-07-19T06:21:48Z

src/domain/request/requestExecutor/UrlRequestExecutor.ts

+import Request from '@core/domain/request/Request';
+import Response from '@core/domain/response/Response';
+
+class UrlRequestExecutor implements RequestExecutor {


Why does RequestExecutor know that it is handling url based request? Maybe moving this knowledge to fetchDataUseCase would be cleaner approach?

It is indeed implemented like this. All url-specific stuff happens in Fetcher.

UrlRequestExecutor was just my attemp of naming a group of ['json', 'xml', 'html', 'ipfs'] dataSources. Do you have an idea for a better name?

src/domain/request/usecase/ExecuteReadyRequestsUseCase.spec.ts

src/domain/common/port/RequestExecutorPort.ts

miss click

…+ other CR changes

src/domain/request/requestExecutor/RequestExecutorFactory.ts

przemyslawjanpietrzak · 2019-07-19T14:01:33Z

src/domain/request/requestExecutor/RequestExecutorFactory.ts

+        selectDataUseCase: SelectDataUseCase,
+        logger: LoggerPort,
+    ) {
+        this.requestExecutors = [new UrlRequestExecutor(fetchDataUseCase, selectDataUseCase, logger)];


you can move it out of constructor

But why? I think havingrequest executors represent state of this class. It doesn't make sense for RequestExecutorFactory to exist if it doesn't have and RequestExecutors.

Are you referring to some JS standard that I'm not aware of?

nvm, my bad, sorry

przemyslawjanpietrzak · 2019-07-19T14:02:04Z

src/domain/request/requestExecutor/RequestExecutorFactory.ts

+    }
+
+    create(contentType: string): RequestExecutor {
+        let chosenRequestExecutor = null;


sounds like find/some array method ;)

this.requestExecutors.find(re => re.canHandle(contentType)) :) . Surprised by JS, this is done even simpler than in Java.

tcichowicz · 2019-07-22T09:17:25Z

src/domain/common/port/RequestExecutorPort.ts

+import Response from '@core/domain/response/Response';
+
+export default interface RequestExecutorPort {
+    execute(request: Request, response: Response): Promise<Response>;


Why would we both accept and return a Response? Is response an in/out param?

It was to conform to existing error handling, but after your comment I got idea on how to design it so that I can remove response param.

tcichowicz · 2019-07-22T09:18:21Z

src/application/dataFetcher/AxiosUrlDataFetcherAdapter.ts

-  async fetch(url): Promise<string> {
+import DataFetcher from '@core/domain/common/port/DataFetcherPort';
+import { HttpError } from '@core/domain/common/utils/error';
+import RequestUrlParser from '@core/domain/request/RequestUrlParser';


Looks like RequestUrlParser is now used in two different places (here and in Response domain object). Should AxiosUrlDataFetcherAdapter internalize this logic instead of calling it?

Yes. In current state, RequestUrlParser implements some features that are distinct to the UrlRequestExecutor. It will be refactored later, as now we need this in master to deliver random data provider.

tcichowicz · 2019-07-22T09:19:12Z

src/domain/request/requestExecutor/RequestExecutorFactory.ts

+        this.requestExecutors = [new UrlRequestExecutor(fetchDataUseCase, selectDataUseCase, logger)];
+    }
+
+    create(contentType: string): RequestExecutor {


Just cosmetic - it's not really a factory but a finder of existing stateless objects. The name just seems a bit misleading when you look at the implementation.

Fair enough ,renaming from Factory to Strategy

tcichowicz · 2019-07-22T15:23:53Z

src/domain/request/usecase/ExecuteReadyRequestsUseCase.ts

    private readonly sendResponseToOracleUseCase: SendResponseToOracleUseCase,
    private readonly requestRepository: RequestRepositoryPort,
    private readonly responseRepository: ResponseRepositoryPort,
+    private readonly requestExecutorFactory: RequestExecutorStrategy,


Naming inconsistency: strategy-factory

refactored data fetchers to be more abstract

7bb92c1

kss-espeo requested review from kwedrowicz and tcichowicz July 3, 2019 13:54

tcichowicz suggested changes Jul 3, 2019

View reviewed changes

change some names that were incorrectly too concrete

d8b4343

przemyslawjanpietrzak reviewed Jul 3, 2019

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

Krzysztof Spisak-Spisacki added 6 commits July 3, 2019 20:18

removed naste .gitignore entry

79845f4

Merge branch 'master' into refactor/abstractonise-data-fetchers

b1827db

added flag that imports paths from tsconfig

6a87ac2

Merge branch 'bug/integration-test-error-after-ts-migration' into ref…

669b0d2

…actor/abstractonise-data-fetchers

tslint fixes and fixed npm run tslint for Windows

a406111

adding a change that somehow was not merged from master

d8bf615

tcichowicz reviewed Jul 5, 2019

View reviewed changes

src/domain/common/port/DataFetcherPort.ts Outdated Show resolved Hide resolved

Krzysztof Spisak-Spisacki added 4 commits July 5, 2019 12:04

Merge branch 'master' into refactor/abstractonise-data-fetchers

f2ec02c

parameter rename that wsa not applied after TS migration

dbd673b

added files that were not referenced to index.ts, because tslint want…

e71cf72

…s so

implemented request executor factory

bfb1f7b

przemyslawjanpietrzak previously approved these changes Jul 8, 2019

View reviewed changes

tcichowicz reviewed Jul 8, 2019

View reviewed changes

src/application/requestExecutor/RequestExecutorFactory.ts Outdated Show resolved Hide resolved

przemyslawjanpietrzak reviewed Jul 8, 2019

View reviewed changes

src/domain/common/port/DataFetcherPort.ts Show resolved Hide resolved

moved request executor to domain

67a2ef8

kss-espeo dismissed przemyslawjanpietrzak’s stale review via 67a2ef8 July 8, 2019 11:17

przemyslawjanpietrzak reviewed Jul 8, 2019

View reviewed changes

src/application/dataFetcher/AxiosUrlDataFetcherAdapter.ts Outdated Show resolved Hide resolved

src/domain/common/usecase/FetchDataUseCase.ts Outdated Show resolved Hide resolved

src/domain/request/requestExecutor/RequestExecutorFactory.ts Outdated Show resolved Hide resolved

fixed peculiar tslint errors

a092881

przemyslawjanpietrzak suggested changes Jul 17, 2019

View reviewed changes

src/domain/request/requestExecutor/RequestExecutorFactory.ts Outdated Show resolved Hide resolved

Krzysztof Spisak-Spisacki added 3 commits July 18, 2019 14:43

using request executors

1097e10

using request executor

da20813

argument types

18a15e9

kwedrowicz reviewed Jul 19, 2019

View reviewed changes

przemyslawjanpietrzak previously approved these changes Jul 19, 2019

View reviewed changes

src/domain/common/port/RequestExecutorPort.ts Outdated Show resolved Hide resolved

kss-espeo requested review from tcichowicz and kwedrowicz July 19, 2019 13:56

making request executors aware of which contentTypes they can handle …

6b2cea1

…+ other CR changes

przemyslawjanpietrzak reviewed Jul 19, 2019

View reviewed changes

kss-espeo mentioned this pull request Jul 19, 2019

Feature/random data provider #42

Merged

simplified request executor choosing + stylistic changes

01ac038

kss-espeo requested a review from przemyslawjanpietrzak July 19, 2019 17:37

tcichowicz reviewed Jul 22, 2019

View reviewed changes

Krzysztof Spisak-Spisacki added 2 commits July 22, 2019 13:17

removed response from request executor interface

9e40809

rename

04f297c

tcichowicz previously approved these changes Jul 22, 2019

View reviewed changes

rename

527fb56

kss-espeo dismissed tcichowicz’s stale review via 527fb56 July 22, 2019 16:27

kss-espeo requested a review from tcichowicz July 22, 2019 16:28

tcichowicz approved these changes Jul 22, 2019

View reviewed changes

kss-espeo merged commit 3ce89ac into master Jul 22, 2019

kss-espeo deleted the refactor/abstractonise-data-fetchers branch July 22, 2019 16:35

kss-espeo mentioned this pull request Jul 23, 2019

Move entire contentType-specific logic into RequestStrategy #47

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactored data fetchers to be more abstract #32

refactored data fetchers to be more abstract #32

kss-espeo commented Jul 3, 2019

tcichowicz Jul 3, 2019

kss-espeo Jul 3, 2019

tcichowicz Jul 3, 2019

kss-espeo Jul 3, 2019 •

edited

tcichowicz Jul 4, 2019 •

edited

przemyslawjanpietrzak left a comment

kwedrowicz Jul 19, 2019

kss-espeo Jul 19, 2019

przemyslawjanpietrzak Jul 19, 2019

kss-espeo Jul 19, 2019

przemyslawjanpietrzak Jul 22, 2019

przemyslawjanpietrzak Jul 19, 2019

kss-espeo Jul 19, 2019 •

edited

tcichowicz Jul 22, 2019

kss-espeo Jul 22, 2019 •

edited

tcichowicz Jul 22, 2019

kss-espeo Jul 22, 2019 •

edited

tcichowicz Jul 22, 2019

kss-espeo Jul 22, 2019 •

edited

tcichowicz Jul 22, 2019

refactored data fetchers to be more abstract #32

refactored data fetchers to be more abstract #32

Conversation

kss-espeo commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kss-espeo Jul 3, 2019 • edited

Choose a reason for hiding this comment

tcichowicz Jul 4, 2019 • edited

Choose a reason for hiding this comment

przemyslawjanpietrzak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kss-espeo Jul 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kss-espeo Jul 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kss-espeo Jul 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kss-espeo Jul 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kss-espeo Jul 3, 2019 •

edited

tcichowicz Jul 4, 2019 •

edited

kss-espeo Jul 19, 2019 •

edited

kss-espeo Jul 22, 2019 •

edited

kss-espeo Jul 22, 2019 •

edited

kss-espeo Jul 22, 2019 •

edited