Filters implementation #33

jfthuillier · 2018-06-08T08:32:06Z

In parseHydraDocumentation > addFilters
I made the choice to ignore filters with null property (PropertyFilter provides null).
Are you ok with this ?

soyuka · 2018-06-13T12:33:17Z

src/hydra/addFilters.js

+    let promise = fetchResource(resource.url).then(response => {
+      const resourceFilters = [];
+
+      if (response.filters) {


if (!response.filters) { return []; } // loop on filters

soyuka · 2018-06-13T12:33:44Z

src/hydra/addFilters.js

+
+          const resourceFilter = new Filter(property, filter.variable);
+
+          resourceFilters.push(resourceFilter);


can be resourceFilters.push(new Filter(property, filter.variable))

soyuka · 2018-06-13T12:34:06Z

src/hydra/addFilters.js

+        for (const filter of response.filters) {
+          let property = filter.property;
+
+          // TODO : To prevent PropertyFilter, maybe should be handle specifically ?


what do you mean?

The PropertyFilter provides null as property, but it can be applied to all properties. I thought i could iterate on each properties to create all filters, but this behavior could be different on another type of filter providing null as property, so i guess just skipping the filter might be what we want.

soyuka · 2018-06-13T12:34:50Z

src/hydra/parseHydraDocumentation.test.js

+  {
+    property: "id",
+    variable: "id"
+  }


possible to add a filter without property to test the line you commented above?

soyuka · 2018-06-13T20:03:39Z

src/hydra/parseHydraDocumentation.test.js

@@ -1365,7 +1365,7 @@ test("Invalid docs JSON", async () => {
 });

 test("Invalid entrypoint JSON", async () => {
-  let entrypoint = `{foo,}`;
+  const entrypoint = `{foo,}`;


you don't need brackets, use a simple quote (') instead :)

This change came from #34. It has been removed.

dunglas · 2018-06-13T21:59:12Z

src/Filter.js

+  variable: string;
+
+  /**
+   * @param {string}        property


Too many spaces?

dunglas · 2018-06-13T22:00:47Z

src/hydra/fetchResource.js

+import get from "lodash.get";
+
+export default async entrypointUrl => {
+  return await fetchJsonLd(entrypointUrl, { items_per_page: 0 }).then(


By default, the name of this attribute in API Platform core is itemsPerPage, we should use it here.

mauchede · 2018-06-15T11:00:18Z

@jfthuillier: To improve readability of this PR, could you rebase your code? It should remove changes from your first PR.

jfthuillier · 2018-06-15T11:44:14Z

I can't manage to squash the last merge commit with the Filters implementation 😡

vdumontier · 2018-06-20T09:26:36Z

Just tested and work fine for me (including api-platform/admin#100)
Needed to add authentification support for api-doc-parser (using fetch option) cause my resources endpoints are protected.
Needed to change fetchResources error to return response, to handle 401 status -> /login redirection.

dunglas · 2018-06-21T08:34:06Z

src/hydra/addFilters.js

+
+      const resourceFilters = [];
+
+      for (const filter of response.filters) {


We usually use the functional style: response.filters.forEach()

dunglas · 2018-06-21T08:35:11Z

src/hydra/addFilters.js

+      const resourceFilters = [];
+
+      for (const filter of response.filters) {
+        let property = filter.property;


Can be const, the variable can even be removed

dunglas · 2018-06-21T08:37:25Z

src/hydra/fetchResource.js

+import fetchJsonLd from "./fetchJsonLd";
+import get from "lodash.get";
+
+export default async entrypointUrl => {


resourceUrl?

src/hydra/parseHydraDocumentation.test.js

dunglas · 2018-06-21T09:03:51Z

src/Filter.js

+/**
+ * @property {string} variable - The variable of this field
+ */
+export default class Filter {


I'm not very confident in this structure.
Here is the OpenAPI structure for filters (they did a great job): https://swagger.io/docs/specification/describing-parameters/
And the Hydra one: http://www.hydra-cg.com/spec/latest/core/#x5-2-templated-links

To support both I suggest to rename this class Parameter with the following structure:

class Parameter { variable: string; type: string; // (an IRI) required: bool; description: string; }

jfthuillier · 2018-06-26T14:00:37Z

src/hydra/addParameters.js

+        }
+
+        resourceParameters.push(
+          new Parameter(parameter.variable, "", parameter.required, "")


@dunglas : what is the type value you are expecting ?

Something like an XSD type?

It will be the same that the range attribute of the Field

Tests fixed & and attribute name changed to range

mauchede · 2018-06-27T07:48:28Z

src/hydra/addParameters.js

+  const promises = [];
+
+  for (const resource of api.resources) {
+    let promise = fetchResource(resource.url).then(response => {


let -> const

mauchede · 2018-06-27T07:51:20Z

src/hydra/addParameters.js

+
+  for (const resource of api.resources) {
+    let promise = fetchResource(resource.url).then(response => {
+      if (!response.parameters) {


You can use a default value:

const resourceParameters = []; const { parameters = {} } = response; parameters.forEach(....); return resourceParameters;

mauchede · 2018-06-27T09:32:51Z

src/hydra/addParameters.js

+      const resourceParameters = [];
+      const { parameters = {} } = response;
+
+      parameters.forEach(parameter => {


You only used parameter.variable, parameter.property and parameter.required. Maybe could you use destructuring?

parameters.forEach(({ property = null, required, variable }) => { if (null === property) { return; } // ... resourceParameter.push( new Parameter(variable, range, required, "") ); });

mauchede · 2018-06-27T09:34:23Z

src/hydra/addParameters.js

+        let range = null;
+
+        const resourceProperty = resource.fields.find(
+          field => property === field.name


({ name }) => property === name

mauchede · 2018-06-27T09:36:48Z

src/hydra/addParameters.js

+        const resourceProperty = resource.fields.find(
+          field => property === field.name
+        );
+        if (resourceProperty) {


const { range = null } = resource.fields.find(...) || {};

mauchede · 2018-06-27T11:31:54Z

src/hydra/addParameters.js

+  const promises = [];
+
+  for (const resource of api.resources) {
+    const promise = fetchResource(resource.url).then(response => {


It seems that only response.parameters is needed.

In that case:

const promise = fetchResource(resource.url).then(({ parameters = {} }) => { const resourceParameters = []; parameters.forEach(({ property = null, required, variable }) => { // ... }); return resourceParameters; });

mauchede · 2018-06-27T11:35:47Z

src/hydra/addParameters.js

+          return;
+        }
+
+        const { range = null } = resource.fields.find(


Is it sure to always match a field here? In my previous comment, I suggested to add a default value (|| {}) to avoid runtime errors.

mauchede · 2018-06-27T11:38:27Z

src/hydra/parseHydraDocumentation.js

+          status: get(response, "status")
+        })
+    )
+    .then(({ api, response, status }) => {


.then(({ api, response, status }) => ( addParameters(api).then(api => ({ api, response, status })); ));

mauchede · 2018-06-27T11:43:55Z

src/hydra/parseHydraDocumentation.js

-      ) {
-        throw new Error(
-          'The entrypoint definition has no "http://www.w3.org/ns/hydra/core#supportedProperty" key or it is not an array.'
+  return fetchEntrypointAndDocs(entrypointUrl, options)


A lot of modifications in this file are only CS changes. Could you remove them from this PR?

Done, is it just to have visibility for your review ?
I will have to reuse yarn fix before merging, right ?

jfthuillier · 2018-07-23T13:18:29Z

@mauchede @dunglas : not sure if you are to busy or the checks failing make you think there is still work to do. But checks are deliberatly failing because of eslint to make your review of parseHydraDocumentation.js easier, when you'll approve these changes i'll apply eslint.

dunglas · 2018-10-06T09:35:46Z

Thanks @jfthuillier! Great PR.

jfthuillier mentioned this pull request Jun 8, 2018

Filters implementation api-platform/admin#100

Merged

jfthuillier changed the title ~~WIP : filters implementation~~ Filters implementation Jun 12, 2018

mauchede added the enhancement label Jun 13, 2018

mauchede assigned dunglas and mauchede Jun 13, 2018

mauchede requested review from dunglas, mauchede and soyuka June 13, 2018 10:59

soyuka requested changes Jun 13, 2018

View reviewed changes

soyuka approved these changes Jun 13, 2018

View reviewed changes

soyuka reviewed Jun 13, 2018

View reviewed changes

dunglas requested changes Jun 13, 2018

View reviewed changes

jfthuillier force-pushed the filters branch from 3116e95 to 09ebf35 Compare June 14, 2018 07:07

jfthuillier force-pushed the filters branch 3 times, most recently from 5751076 to 99d112b Compare June 15, 2018 11:26

jfthuillier force-pushed the filters branch from 99d112b to 9df485a Compare June 21, 2018 08:51

dunglas requested changes Jun 21, 2018

View reviewed changes

jfthuillier force-pushed the filters branch 6 times, most recently from 3101b8a to 4757139 Compare June 26, 2018 13:55

jfthuillier commented Jun 26, 2018

View reviewed changes

jfthuillier force-pushed the filters branch 2 times, most recently from 286a937 to d77fd6a Compare June 26, 2018 16:04

mauchede reviewed Jun 27, 2018

View reviewed changes

jfthuillier force-pushed the filters branch from b019eca to 2a0bf2a Compare June 27, 2018 13:23

dunglas approved these changes Oct 6, 2018

View reviewed changes

soyuka approved these changes Oct 6, 2018

View reviewed changes

JF and others added 2 commits October 6, 2018 11:25

Filters implementation

c879e87

Fix CS

aec146f

dunglas force-pushed the filters branch from 2a0bf2a to aec146f Compare October 6, 2018 09:33

dunglas merged commit da3131b into api-platform:master Oct 6, 2018

olivermack mentioned this pull request Dec 14, 2018

Extensive API calls due to filter preparation #42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filters implementation #33

Filters implementation #33

jfthuillier commented Jun 8, 2018 •

edited

soyuka Jun 13, 2018

soyuka Jun 13, 2018

soyuka Jun 13, 2018

jfthuillier Jun 13, 2018

soyuka Jun 13, 2018

soyuka Jun 13, 2018

mauchede Jun 14, 2018

dunglas Jun 13, 2018

dunglas Jun 13, 2018

jfthuillier Jun 15, 2018

mauchede commented Jun 15, 2018 •

edited

jfthuillier commented Jun 15, 2018

vdumontier commented Jun 20, 2018

dunglas Jun 21, 2018

dunglas Jun 21, 2018

dunglas Jun 21, 2018

dunglas Jun 21, 2018

jfthuillier Jun 26, 2018

dunglas Jun 26, 2018

jfthuillier Jun 26, 2018

jfthuillier Jun 26, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

mauchede Jun 27, 2018

jfthuillier Jun 27, 2018

jfthuillier commented Jul 23, 2018

dunglas commented Oct 6, 2018


		const resourceFilter = new Filter(property, filter.variable);

		resourceFilters.push(resourceFilter);


		const resourceFilters = [];

		for (const filter of response.filters) {

Filters implementation #33

Filters implementation #33

Conversation

jfthuillier commented Jun 8, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauchede commented Jun 15, 2018 • edited

jfthuillier commented Jun 15, 2018

vdumontier commented Jun 20, 2018

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

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

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

jfthuillier commented Jul 23, 2018

dunglas commented Oct 6, 2018

jfthuillier commented Jun 8, 2018 •

edited

mauchede commented Jun 15, 2018 •

edited