feathersjs-ecosystem / feathers-sync

Synchronize service events between Feathers application instances
MIT License
221 stars 41 forks source link

Converting circular structure to JSON #195

Open palmtown opened 8 months ago

palmtown commented 8 months ago

Steps to reproduce

(First please check that this issue is not already solved as described here)

Uncaught error at https://github.com/feathersjs-ecosystem/feathers-sync/blob/release/lib/core.js#L62 that causes feathersjs to return a 500 error although the request completed successfully, with exception that the sync failed. In short, the mongoose object is now set at context.arguments[1].mongoose and triggers this error as it contains circular structures.

Expected behavior

No errors converting object to JSON.

Actual behavior

Tell us what happens instead

Error is generated at https://github.com/feathersjs-ecosystem/feathers-sync/blob/release/lib/core.js#L62 due to context.arguments[1].mongoose.

System configuration

Tell us about the applicable parts of your setup.

Module versions (especially the part that's not working): 3.0.3

NodeJS version: 18

Operating System: Ubuntu

Browser Version:

React Native Version:

Module Loader:

daffl commented 8 months ago

If you payload contains circular JSON you can use a custom serializer

palmtown commented 8 months ago

Hello @daffl

Thanks for providing the custom serializer solution; I can use that to solve my problem. Nonetheless, are you aware that the mongoose object is now set in context.argument[1].mongoose which includes circular structures? Note that this error was not triggering previously. Also, I am not setting this in my code.

Also, if newer versions now set context.argument[1].mongoose, do you believe a fix to feathers-sync would be the appropiate resolution vs. having developers implement a customer serializer?

Again, thanks for the recommended solution, I will use that.

daffl commented 8 months ago

Did you pass the Mongoose object somewhere? Unless explicitly done so, I don't believe there should be circular objects in the context.

palmtown commented 8 months ago

Hello @daffl

Good question! I do not believe I have done that. However, I'm going to review my code and commits to double check and be sure. As you may know, the context.argument[1] object seems to be variables set by feathers such as query, headers, provider etc. whereas context.argument[0] contains the data that I set.

I'll dig a little deeper and see what I can find out.

jordandenison commented 4 months ago

@daffl I am getting this error as well when trying to authenticate with a custom auth strategy. Happens for both authentication.create and users.patch. I'm not sure how to or if using a custom serializer would work in this case but putting delete context.params.resolve right before the emit in core.js seems to fix the issue. Do you have any idea why this might be happening, and do you forsee any issues with removing this property before emitting?

daffl commented 4 months ago

Hm, yes, that might indeed have to be excluded.

jordantelus commented 4 months ago

Rut roh. Any suggestions? I have also tried delete context.params.resolve.originalContext but the issue still occurs.

daffl commented 4 months ago

Serializing it without the property should work:

app.configure(
  sync({
    uri: 'redis://localhost:6379',
    serialize: (payload) => {
      const { context } = payload
      const { resolve, ...params } = context.params

      return JSON.stringify({
        ...payload,
        context: {
          ...context,
          params
        }
      })
    }
  })
);
jordantelus commented 4 months ago

Hmm, that code still gives the error yet this does not:

    serialize: (payload) => {
      delete payload.context.params.resolve
      return JSON.stringify(payload)
    }

Which does not make a whole lot of sense. But knowing that it is safe to exclude the resolve property is good to know. Thank you!

daffl commented 4 months ago

That does seem strange since it should really just do the same thing.

jordantelus commented 4 months ago

Yes. Exactly. I am super puzzled as well. lol

pedobry commented 3 months ago

There is also resolve once more in the context.arguments[1]. What works for me:

  serialize: (payload) => {
      // use custom serializer to remove context.params.resolve and
      // context.arguments from the payload
      // otherwise, it will throw an error when trying to serialize the payload
      // due to circular structure
      const { context } = payload;
      const { resolve, ...params } = context.params;

      return JSON.stringify({
          ...payload,
          context: {
              ...context,
              arguments: [],
              params,
          },
      });
  },